From c2eecba786d3a3c74f2c8bca99f1a9cf02e6e7e9 Mon Sep 17 00:00:00 2001 From: Jia Hao Date: Wed, 4 Oct 2023 02:29:17 +0000 Subject: [PATCH] [Impeller] Don't define `CanvasRecorder` if `IMPELLER_TRACE_CANVAS` is not set. (flutter/engine#46476) Internal bug: b/303067268 https://github.com/flutter/engine/pull/46376 is causing a breakage to the internal engine build because of https://github.com/flutter/engine/blob/543348a044bf0b64675371aee5f39d1e823e423d/impeller/aiks/canvas_recorder.h#L58-L62. Internally, we do not set `IMPELLER_TRACE_CANVAS`. It looks like the cause is that the internal toolchain causes the `static_assert` to be compiled even though the template is not instantiated. @chingjun helped me to figure out the following: https://stackoverflow.com/questions/5246049/c11-static-assert-and-template-instantiation points us to the spec. In the later version (ISO/IEC 14882:2017(E)): > The program is ill-formed, no diagnostic required, if ... no valid specialization can be generated for a template or a substatement of a constexpr if statement (9.4.1) within a template and the template is not instantiated,
The relevant section ![The relevant section of the spec](https://github.com/flutter/engine/assets/7111741/4503efcd-9479-4c7a-b4a1-7302dea1653b)
Interpretation: the compiler can either choose to emit the error caused by the `static_assert` or not. Currently the compiler used by the build here on LUCI does not; internally it does. For example, the following links shows that simply changing the Clang version affects whether the error appears or not for a minimal template. - ok: https://godbolt.org/z/n9nYrcvcP - not ok: https://godbolt.org/z/fWcvdcn35 Hence, `#ifdef` out the class instead of using a `static_assert` for more consistent behavior across these two toolchains. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- engine/src/flutter/impeller/aiks/canvas_recorder.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/engine/src/flutter/impeller/aiks/canvas_recorder.h b/engine/src/flutter/impeller/aiks/canvas_recorder.h index 56601207f8..62711c13fc 100644 --- a/engine/src/flutter/impeller/aiks/canvas_recorder.h +++ b/engine/src/flutter/impeller/aiks/canvas_recorder.h @@ -47,6 +47,9 @@ enum CanvasRecorderOp : uint16_t { DrawAtlas, }; +// Canvas recorder should only be used when IMPELLER_TRACE_CANVAS is defined +// (never in production code). +#ifdef IMPELLER_TRACE_CANVAS /// Static polymorphic replacement for impeller::Canvas that records methods /// called on an impeller::Canvas and forwards it to a real instance. /// TODO(https://github.com/flutter/flutter/issues/135718): Move this recorder @@ -55,12 +58,6 @@ enum CanvasRecorderOp : uint16_t { template class CanvasRecorder { public: -#ifndef IMPELLER_TRACE_CANVAS - // Canvas recorder should only be used when IMPELLER_TRACE_CANVAS is defined - // (never in production code). - static_assert(false); -#endif - CanvasRecorder() : canvas_() { serializer_.Write(CanvasRecorderOp::New); } explicit CanvasRecorder(Rect cull_rect) : canvas_(cull_rect) { @@ -286,5 +283,6 @@ class CanvasRecorder { Canvas canvas_; Serializer serializer_; }; +#endif } // namespace impeller