[PATCH] D153969: [clang][ExprConstant] Fix crash on uninitialized base class subobject

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet 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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153969/new/

https://reviews.llvm.org/D153969