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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
