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<
----------------
cor3ntin wrote:
> 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)
Yeah, I think this is a case where it's better to just mention the option but
not suggest a value.
================
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:
> 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.
SGTM!
================
Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc-limits.cpp:1
+// RUN: %clang_cc1 -std=c++2a -verify -fconstexpr-steps=1024 %s
+
----------------
aaron.ballman wrote:
>
Should we add -Wvla to show when constant folding fails we're getting a VLA?
================
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}} \
----------------
cor3ntin wrote:
> 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.
Ahhh, I see. Hmm, okay, not as bad as I had thought.
================
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)'}} \
----------------
cor3ntin wrote:
> 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
I think that'd be a good test to add as well, yes,
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155955/new/
https://reviews.llvm.org/D155955
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
