kadircet added a comment.
thanks this looks amazing! i am also not an expert in these parts of the code
but AFAICT the proposed behavior is in line with the contract. i am a little
worried about the cost of extra copy (especially when there are only a handful
of elements), but it should be probably fine.
================
Comment at: clang/lib/AST/ExprConstant.cpp:10617
+ // copying the data, which is wasteful.
+ for (const unsigned N : {1u, FinalSize}) {
+ unsigned OldElts = Value->getArrayInitializedElts();
----------------
maybe unroll the loop with a helper like `checkConstantInitialization` and just
call it for a single element first and perform copy & call again for the whole
thing ?
that way we can migrate performance issues later on (e.g. put a threshold on
`FinalSize`, don't perform the small step check if it's less than 100, since
copying might introduce a significant extra latency in such cases).
WDYT?
================
Comment at: clang/lib/AST/ExprConstant.cpp:10641
+ return false;
+ if (Info.EvalStatus.Diag && !Info.EvalStatus.Diag->empty() &&
+ !Info.keepEvaluatingAfterFailure())
----------------
i am a little worried about having "non fatal/error" diagnostics here. But
looking at the documentation of the field, intention seems to be making this
empty iff evaluation succeeded so far. so it's probably fine, but might be
worth introducing in a separate patch to make the revert easy, in case it
breaks things (+ this seems to be an orthogonal change to the rest)
================
Comment at: clang/test/SemaCXX/PR51712-large-array-constexpr-check-oom.cpp:2
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
----------------
maybe test with `ulimit` and ~1 GB of memory? (you might need `Requires: shell`
and `unsupported: windows`.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113120/new/
https://reviews.llvm.org/D113120
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
