[patch] Improve diagnostic on default-initializing const variables (PR20208)

[Richard Smith]

On Wed, Jul 9, 2014 at 2:48 PM, Nico Weber <thakis at chromium.org> wrote:

> Hi,
>
> the attached patch attempts to fix PR20208 in the way I understood
> Richard's suggestion in comment 1.
>
> The patch is a bit hairy (but short) since it adds a fixit on an error
> emitted by InitializationSequence – this means it needs to build a correct
> AST, which in turn means InitializationSequence::Failed() cannot return
> true when this fixit is applied. As a workaround, the patch adds a fixit
> member to InitializationSequence, and InitializationSequence::Perform()
> prints the diagnostic if the fixit member is set right after its call to
> Diagnose. I think that function is usually called when
> InitializationSequences are used – InitListChecker::PerformEmptyInit()
> doesn't call it though, so there might be bugs there.
>

The InitListChecker case never performs default-initialization, so I think
this is technically OK. (I was also worried about how this would behave in
overload resolution, but we get away with that because overload resolution
never considers default initialization of variables.)

Please add this as a testcase:

  template<typename T> decltype(new const T) f(T);
  int &f(...);

  int &k = f(0); // ok, calls non-template

(I think this test would fail if you extended the current approach to cover
the initialization of any entity. You could fix that by only doing the
recovery outside of SFINAE contexts.)


+  if (!ZeroInitializationFixit.empty()) {
+    // The initialization would have succeeded with this fixit. Since the
fixit
+    // is on the error, we need to build a valid AST in this case, so this
isn't
+    // handled in the Failed() branch above.
+    QualType DestType = Entity.getType();
+    S.Diag(Kind.getLocation(), diag::err_default_init_const)
+        << DestType << (bool)DestType->getAs<RecordType>()
+        << FixItHint::CreateInsertion(ZeroInitializationFixitLoc,
+                                      ZeroInitializationFixit);
+  }

It might be more natural to emit this diagnostic as part of performing the
SK_ZeroInitialization step.

Ideas? Easiest would be to put the fixit on a note, but the note would just
> repeat the snippet from the diag itself, which isn't great.
>

We do that in a lot of cases; this wouldn't be so bad, and it'd be safer.
The current approach is also workable, because we never make a choice based
on whether default initialization of a variable is valid, but it seems
fragile.

The nice thing about putting it in a separate note is that you can point
the caret directly at the place where the fixit text should be inserted,
while still having the main diagnostic point at the entity being
initialized; that tends to make fixits much more obvious.


> Or we could omit the fixit and just do the wording change, but the fixit
> makes the diagnostic much clearer.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140716/300831f2/attachment.html>