aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:2422
+ << BS.getType();
+ Info.Note(BS.getBeginLoc(),
diag::note_constexpr_base_inherited_here);
+ return false;
----------------
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
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153969/new/
https://reviews.llvm.org/D153969
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
