[PATCH] D130791: [clang] Short-circuit trivial constexpr array constructors

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D130791#3692071 <https://reviews.llvm.org/D130791#3692071>, @dblaikie wrote:

> also: What about zero length arrays? (which are supported as an extension) - this change would cause failures that wouldn't be emitted in the old code?

There's a `if (FinalSize == 0) return true;` case above, so this code will never be reached for zero-length arrays. That loop here is pretty confusing... `OldElts` is either zero (for the first iteration), or one (for the second iteration, where `N == FinalSize`). I tried to untangle this, but it doesn't work very well since the `APValue` we pass to `VisitCXXConstructExpr()` must be in an array `APValue`...

As for testing: I didn't include a test because of what you two mentioned, I can't really test that a test case finished in under X seconds...



================
Comment at: clang/lib/AST/ExprConstant.cpp:10836-10838
+    bool HasTrivialConstructor = CheckTrivialDefaultConstructor(
+        Info, E->getExprLoc(), E->getConstructor(),
+        E->requiresZeroInitialization());
----------------
aaron.ballman wrote:
> The big question this raises for me is: will this cause constexpr to fail because of the note diagnostics when the type does not have a trivial default constructor? Or does this just bump the failure up a bit so that we fail before we start walking over the array elements?
I can't come up with an example that would make this fail, or fail differently than it did before. For a constructor that is not marked `constexpr` and where `CheckTrivialDefaultConstructor` returns `false`, the first `VisitCXXConstructExpr` will return `false` and no diagnostic will be emitted. In the cases I tried, `Info.EvalStatus.Diag` is `nullptr` anyway, so yeah. If it did emit a diagnostic, I would assume that the failure just happens a little early, yes.

The alternative would be to try to integrate the `CheckTrivialDefaultConstructor` call into the loop, but I'm not a fan of that loop anyway :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130791/new/

https://reviews.llvm.org/D130791