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;
----------------
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.
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
