Page MenuHomePhabricator

Add diagnostics to require_constant_initialization
ClosedPublic

Authored by EricWF on Sep 8 2016, 8:03 PM.

Details

Summary

This hooks up the detailed diagnostics why constant initialization was not possible if require_constant_initialization reports an error. I have updated the test to account for the new notes.
Everything works fine, except that in C++11 mode we get:

error: 'note' diagnostics expected but not seen:
  File /data/llvm/tools/clang/test/SemaCXX/attr-require-constant-initialization.cpp Line 229 (directive at /data/llvm/tools/clang/test/SemaCXX/attr-require-constant-initialization.cpp:231): non-constexpr constructor 'NonLit' cannot be used in a constant expression
error: 'note' diagnostics seen but not expected:
  (frontend): non-literal type 'NonLit' cannot be used in a constant expression

This is because of an ImplicitValueInitExpr that gets passed into CheckLiteralType, but since ImplicitValueInitExpr doesn't have source information we get an invalid source location. I'm not really sure how to fix that (Is it possible to test for a note without source location?). Adding the proper source locations to ImplicitValueInitExpr seemed like a bigger undertaking than was warranted for this patch, so I'd appreciate guidance on how to proceed.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 70775.Sep 8 2016, 8:03 PM
loladiro retitled this revision from to Add diagnostics to require_constant_initialization.
loladiro updated this object.
loladiro added reviewers: EricWF, aaron.ballman, rsmith.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: cfe-commits.
rsmith edited edge metadata.Sep 9 2016, 1:03 PM

There's no way for -verify to test for a diagnostic with an invalid location at the moment. Maybe add a FIXME and disable that portion of the test for C++11?

lib/Sema/SemaDecl.cpp
10535–10538 ↗(On Diff #70775)

Can you capture the diagnostics from checkInitIsICE instead of recomputing them here? (It looks straightforward to add an overload that takes a vector of notes.) In the non-C++11 case, you can produce a note pointing to CacheCulprit instead.

loladiro added inline comments.Sep 9 2016, 1:07 PM
lib/Sema/SemaDecl.cpp
10535–10538 ↗(On Diff #70775)

The problem is that the result of checking is cached internally and the diagnostics are only produced the first time. I don't think it's necessarily guaranteed that the above checkInitIsICE is the first such call (unless you can see such a reason).

rsmith added inline comments.Sep 9 2016, 1:36 PM
lib/Sema/SemaDecl.cpp
10535–10538 ↗(On Diff #70775)

That's unfortunate, but yeah, it's not reasonable to rely on that. Please just use Init instead of cast<Expr>(var->ensureEvaluatedStmt()->Value) here (ensureEvaluatedStmt() really ought to be a private member of VarDecl...).

loladiro updated this revision to Diff 70915.Sep 9 2016, 3:10 PM
loladiro edited edge metadata.

Address review comments:

  • Disable C++11 test that lacks source locations
  • Use CacheCulprit to give diagnostics for < C++11
EricWF edited edge metadata.Oct 7 2016, 5:14 PM

@rsmith ping. Any more comments on this patch?

@loladiro The patch doesn't apply correctly to the test. Do you mind if I hijack this and fix it?

EricWF commandeered this revision.May 31 2017, 5:30 PM
EricWF updated this revision to Diff 100949.
EricWF edited reviewers, added: loladiro; removed: EricWF.
  • Fix patch so it applies cleanly.
EricWF accepted this revision.May 31 2017, 5:32 PM

LGTM. @loladiro Would you like to commit this?

This revision is now accepted and ready to land.May 31 2017, 5:32 PM
loladiro edited edge metadata.Jun 1 2017, 10:52 AM

Sure, I'll commit it.

This revision was automatically updated to reflect the committed changes.