[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

Sat May 20 10:06:33 PDT 2023

hazohelet added a comment.

In D146358#4357276 <https://reviews.llvm.org/D146358#4357276>, @erichkeane wrote:

> In D146358#4357106 <https://reviews.llvm.org/D146358#4357106>, @hazohelet wrote:
>
>> Thanks for the revert!
>>
>> It seems that the problem is around the list initializer in template variable definition.
>> Minimum reproducible example:
>> `template <typename T> constexpr T foo{};`
>>
>> When the `TextNodeDumper` enabled through `-ast-dump` visits a constexpr `VarDecl` with initializer, it evaluates the `VarDecl` and prints the value if evaluation comes successful.
>> (https://github.com/llvm/llvm-project/blob/55903151a2a505284ce3fcd955b1d394df0b55ea/clang/lib/AST/TextNodeDumper.cpp#L1825)
>> The `VarDecl::evaluateValue()` does not show the notes generated during its failed evaluation. The uninitialized-subobject note is actually generated BEFORE this patch when compiling the reproducers with `-ast-dump` flag on, but is not shown to the user.
>> The message should not be generated here and I assume this is an internal bug that we have to fix before resubmitting this patch.
>
> It seems to me that perhaps the assert you added is not appropriate?  Instead, could we have the OLD diagnostic 'back' as an alternate form (when this is empty)?

If we use the old diagnostic, `subobject of type 'const T' is not initialized` will be generated internally when clang dumps AST of the reproducer `template<typename T> constexpr T foo{};`. This is not what we want, right? I want to keep the assert here because it seems to be a nice bug catcher so far.

I did some more investigation and now I am sure that `TextNodeDumper` should not evaluate the initializer of constexpr-qualified `VarDecl` if it is an variable template definition(in the code I provided as permalink to GitHub). This approach fixes the assertion failure in the regression.
I opened another differential D151033 <https://reviews.llvm.org/D151033> for that change, and I think it would be safe to reland this patch once the other is landed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358