[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:356
+  "cannot allocate array; evaluated array bound %0 exceeds the limit (%1); "
+  "use -fconstexpr-steps=%0 to increase this limit">;
 def note_constexpr_new_too_small : Note<
----------------
I think use of %0 twice here is a bit of a surprise. The first time it's telling you the array bounds, but the second time it's telling you a step limit; these are different things (array bounds contribute to the step limit but there's no reason to assume that setting the step limit to the array bounds will fix anything (or even be larger than the current step limit).


================
Comment at: clang/lib/AST/ExprConstant.cpp:1039
+      // of arrays to avoid exhausting the system resources, as initialization
+      // of each element in likely to take some number of steps anyway.
+      uint64_t Limit = Ctx.getLangOpts().ConstexprStepLimit;
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:6544-6546
+    if (Size && Size > Value.getArrayInitializedElts()) {
       expandArray(Value, Value.getArraySize() - 1);
+    }
----------------
Unnecessary changes.


================
Comment at: clang/lib/AST/ExprConstant.cpp:6754-6758
   bool IsNothrow = false;
   for (unsigned I = 1, N = E->getNumArgs(); I != N; ++I) {
     EvaluateIgnoredValue(Info, E->getArg(I));
     IsNothrow |= E->getType()->isNothrowT();
   }
----------------
cor3ntin wrote:
> Note that that the computation for nothrow is incorrect
Good catch -- are you planning to fix in a follow-up? If not, can you file an issue so we don't lose track of this? (Bonus points for a test case showing that we get this wrong.)


================
Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc-limits.cpp:1
+// RUN: %clang_cc1 -std=c++2a -verify -fconstexpr-steps=1024 %s
+
----------------



================
Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc-limits.cpp:52
+constexpr S<std::size_t> s4(1024); // expected-error {{constexpr variable 's4' must be initialized by a constant expression}} \
+                                   // expected-note@#construct {{constexpr evaluation hit maximum step limit; possible infinite loop?}} \
+                                   // expected-note@#construct_call {{in call}} \
----------------
The wording of this note is a bit unfortunate given that there's no loop in sight. :-(


================
Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc-limits.cpp:59
+constexpr S<std::size_t> s5(1025); // expected-error{{constexpr variable 's5' must be initialized by a constant expression}} \
+                                   // expected-note@#alloc {{cannot allocate array; evaluated array bound 1025 exceeds the limit (1024); use -fconstexpr-steps=1025 to increase this limit}} \
+                                   // expected-note@#call {{in call to 'this->alloc.allocate(1025)'}} \
----------------
If you set the limit to 1025 do you actually succeed?


================
Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc-limits.cpp:79-82
+
+
+
+}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155955