aaron.ballman added inline comments.
================ Comment at: clang/docs/ReleaseNotes.rst:382-383 on overload resolution, when the actual reason for the failure is loss of other qualifiers. +- Clang contexpr evaluator now displays notes as well as an error when a constructor + of base class is not called in the constructor of its derived class. ---------------- ================ Comment at: clang/lib/AST/ExprConstant.cpp:2422 + << BS.getType(); + Info.Note(BS.getBeginLoc(), diag::note_constexpr_base_inherited_here); + return false; ---------------- 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? ================ Comment at: clang/test/SemaCXX/constexpr-subobj-initialization.cpp:30 +} // namespace baseclass_uninit + ---------------- I'd also like to see a test case with more interesting base class specifiers: ``` struct Foo { constexpr Foo(); }; struct Bar : protected Foo { int i; constexpr Bar() : i(12) {} } template <typename Ty> struct Baz { constexpr Baz(); }; struct Quux : Baz<Foo>, private Bar { int i; constexpr Quux() : i(12) {} } ``` 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