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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 2 inline comments as done.
cor3ntin 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<
----------------
aaron.ballman wrote:
> 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).
"use '-fconstexpr-steps' to increase this limit" then?

(The idea was that fconstexpr-steps needs to be at least that value, i agree that the program could still fail a few instructions later)


================
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();
   }
----------------
aaron.ballman wrote:
> 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.)
I'll file an issue because it's unclear to me whether this is the only problem with nothrow allocation, or how they should work in general.
Assuming we fix that `(T*) new(TOO_BIG, nothrow)` still complain about casting a `void*` ptr.

Ie, i already looked into it and decided it was way out of scope.


================
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}} \
----------------
aaron.ballman wrote:
> The wording of this note is a bit unfortunate given that there's no loop in sight. :-(
Line 32. ie, that the case where the allocation succeeds but we can't do much to the array.


================
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)'}} \
----------------
aaron.ballman wrote:
> If you set the limit to 1025 do you actually succeed?
Yes... but no, see previous comment :)
Maybe I can add `constexpr S<std::size_t> s4(100);` that would have no error at all


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155955