Roll clang further, fix buggy test #23751
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| if (!filter_) { | ||
| set_paint_bounds(child_bounds); | ||
| return; |
There was a problem hiding this comment.
This avoids https://github.com/flutter/engine/pull/23751/files#diff-ed00eb358e01399f0680370efff68aa55b41d3a886d3a0d9c9c7078323660a37R55, which previously would get hit in the ImageFilterLayerTest.EmptyFilter test from flow_unittests
There was a problem hiding this comment.
(line 55 below here, 52 in the original source)
|
Whether contents are filtered ought to be immaterial to whether they are eligible for cache substitution. We can perform a filter null check here instead. |
|
We'd have to check in both branches, or at least.juat check in the else branch right? Unless you're saying we should always cache if the filter is null. My thought was that if the filter is null this layer itself is basically just a container layer and we don't need any additional special logic to cache it. |
|
Or is it that this change could miss removing a cache entry if the filter were nulled out between prerolls? |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
My thought was that if the filter is null this layer itself is basically just a container layer and we don't need any additional special logic to cache it.
Fair point.
3 participants
Rolls us to the latest integration Clang toolchain from Fuchsia. Still 12.0.0.
It's really not clear to me why this test didn't segfault before - it seems like it's pretty obviously dereferencing a null pointer.
This is a slight change in behavior though: previously, an image filter layer with a nullptr image filter could end up in the rastercache, whereas no it will not. I think that is more desirable, but hoping @flar can help determine that.