aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
Comment at: clang/lib/AST/ExprConstant.cpp:2422
+ << BS.getType();
+ Info.Note(BS.getBeginLoc(),
diag::note_constexpr_base_inherited_here);
+ return false;
----------------
hazohelet wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > hazohelet wrote:
> > > > > aaron.ballman wrote:
> > > > > > hazohelet wrote:
> > > > > > > hazohelet wrote:
> > > > > > > > tbaeder wrote:
> > > > > > > > > Can you pass `<< BS.getSourceRange()` here? Does that improve
> > > > > > > > > things?
> > > > > > > > Currently, `DiagLoc` points to the variable declaration and the
> > > > > > > > `BS.getSourceRange` covers the line where the base class is
> > > > > > > > inherited. This causes distant source range and thus
> > > > > > > > unnecessarily many lines of snippet printing.
> > > > > > > > e.g.
> > > > > > > > ```
> > > > > > > > struct Base {
> > > > > > > > Base() = delete;
> > > > > > > > };
> > > > > > > > struct Derived : Base {
> > > > > > > > constexpr Derived() {}
> > > > > > > > };
> > > > > > > > constexpr Derived dd;
> > > > > > > > ```
> > > > > > > > Output:
> > > > > > > > ```
> > > > > > > > source.cpp:7:19: error: constexpr variable 'dd' must be
> > > > > > > > initialized by a constant expression
> > > > > > > > 7 | constexpr Derived dd;
> > > > > > > > | ^~
> > > > > > > > source.cpp:7:19: note: constructor of base class 'Base' is not
> > > > > > > > called
> > > > > > > > 7 | struct Derived : Base {
> > > > > > > > | ~~~~
> > > > > > > > 8 | constexpr Derived() {}
> > > > > > > > 9 | };
> > > > > > > > 10 | constexpr Derived dd;
> > > > > > > > | ^
> > > > > > > > source.cpp:4:18: note: base class inherited here
> > > > > > > > 4 | struct Derived : Base {
> > > > > > > > | ^
> > > > > > > > ```
> > > > > > > > (The line numbers seem incorrect but is already reported in
> > > > > > > > https://github.com/llvm/llvm-project/issues/63524)
> > > > > > > >
> > > > > > > > I think we don't need two notes here because the error is
> > > > > > > > already pointing to the variable declaration. Having something
> > > > > > > > like the following would be succint.
> > > > > > > > ```
> > > > > > > > source.cpp:7:19: error: constexpr variable 'dd' must be
> > > > > > > > initialized by a constant expression
> > > > > > > > 7 | constexpr Derived dd;
> > > > > > > > | ^~
> > > > > > > > source.cpp:4:18: note: constructor of base class 'Base' is not
> > > > > > > > called
> > > > > > > > 4 | struct Derived : Base {
> > > > > > > > | ^~~~
> > > > > > > > ```
> > > > > > > > Providing source range would be beneficial because the
> > > > > > > > inherited class often spans in a few lines (the code in the
> > > > > > > > crashing report, for example)
> > > > > > > Sorry, I was looking at the line above. The distant source range
> > > > > > > problem doesn't occur.
> > > > > > >
> > > > > > > I tested another input
> > > > > > > ```
> > > > > > > struct Base {
> > > > > > > Base() = delete;
> > > > > > > constexpr Base(int){}
> > > > > > > };
> > > > > > >
> > > > > > > struct Derived : Base {
> > > > > > > constexpr Derived() {}
> > > > > > > constexpr Derived(int n): Base(n) {}
> > > > > > > };
> > > > > > >
> > > > > > > constexpr Derived darr[3] = {1, Derived(), 3};
> > > > > > > ```
> > > > > > > expecting that the `DiagLoc` points to the second initializer
> > > > > > > `Derived()`, but it pointed to the same location as the error, so
> > > > > > > I'm still in favor of the idea of having a single note here.
> > > > > > Erich's suggestion in
> > > > > > https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607415201
> > > > > > was to continue to evaluate the constructor because there may be
> > > > > > further follow-on diagnostics that are relevant and not related to
> > > > > > the base class subobject. I tend to agree -- is there a reason why
> > > > > > you're not doing that here?
> > > > > My question
> > > > > (https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607177233)
> > > > > was whether or not we should utilize constant evaluator even if the
> > > > > evaluated expression is a semantically-invalid constructor like the
> > > > > crashing case.
> > > > > So in my understanding, Erich's suggestion was that we should
> > > > > continue utilizing the constant evaluator in these cases, and
> > > > > stopping the evaluator here at uninitialized base class subobject is
> > > > > something else.
> > > > Our usual strategy is to continue compilation to try to find follow-on
> > > > issues. For example: https://godbolt.org/z/qrMchvh1f -- even though the
> > > > constructor declaration is not valid, we still go on to diagnose issues
> > > > within the constructor body.
> > > This is the only outstanding discussion that I see left in the review,
> > > and it may be due to a misunderstanding. Am I correct that your changes
> > > cause us to not diagnose further issues in the constructor body because
> > > we're stopping here at the uninitialized base class subobject? e.g., do
> > > we still diagnose this https://godbolt.org/
> > Oops, I munged that link. I meant this one: https://godbolt.org/z/qrMchvh1f
> I don't think it's correct. This patch does not change clang's behavior for
> the code in that link.
> This patch does not introduce any change in diagnostic behavior from clang 16
> (the previous patch is NOT included) other than the wording. Clang16 was also
> stopping the interpreter here. Links below are clang 16 codes.
>
> https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2395-L2399
> Clang 16 calls `CheckEvaluationResult` here.
> https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2355-L2361
> And the `CheckEvaluationResult` called there immediately returns false if
> `Value.hasValue()` evaluates to false.
>
> This patch only adds this `Value.hasValue()` check here before calling
> `CheckEvaluationResult` to emit more precise diagnostics and fix the
> regression. It does nothing more than that.
Ah thank you for the explanation!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153969/new/
https://reviews.llvm.org/D153969
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
