hazohelet added a comment.

@aaron.ballman 
Thanks for the review! I'll add a release note.



================
Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+    if (SubobjectDecl) {
+      Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+      Info.Note(SubobjectDecl->getLocation(),
+                diag::note_constexpr_subobject_declared_here);
+    }
----------------
aaron.ballman wrote:
> Hmm, this breaks one of the contracts of our constexpr evaluation engine, 
> doesn't it? My understanding is that if constexpr evaluation fails, we will 
> have emitted a note diagnostic for why it failed. But if the caller doesn't 
> pass a nonnull `SubobjectDecl`, we'll return `false` but we won't issue a 
> diagnostic.
> 
> I'm surprised no tests lost notes as a result of this change, that suggests 
> we're missing test coverage for the cases where nullptr is passed in 
> explicitly to this function.
Yeah, I was looking into when `SubobjectDecl` can be null here. I `assert`ed 
the non-nullness of `SubobjectDecl` before and found that there exists two 
lines of code 
(https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
 and 
https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
 in the test codes that nulls here.
It seems they are doing the same thing, doing comparison against a pointer to a 
`[[gnu::weak]]` member. A simple reproducing code is here: 
https://godbolt.org/z/qn997n85n
As you can see from the compiler explorer, there's no note emitted here before 
the patch.
I inserted some `printf` into the code before this patch  and confirmed 
`Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << Type` was 
actually called when compiling the reproducing code and that somehow it is 
ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.


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

https://reviews.llvm.org/D146358

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to