<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 9, 2014 at 2:48 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>the attached patch attempts to fix PR20208 in the way I understood Richard's suggestion in comment 1.</div>
<div><br></div><div>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.</div>
</div></blockquote><div><br></div><div>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.)</div>
<div><br></div><div>Please add this as a testcase:</div><div><br></div><div>  template<typename T> decltype(new const T) f(T);</div><div>  int &f(...);</div><div><br></div><div>  int &k = f(0); // ok, calls non-template</div>
<div><br></div><div>(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.)</div><div><br></div>
<div><br></div><div><div>+  if (!ZeroInitializationFixit.empty()) {</div><div>+    // The initialization would have succeeded with this fixit. Since the fixit</div><div>+    // is on the error, we need to build a valid AST in this case, so this isn't</div>
<div>+    // handled in the Failed() branch above.</div><div>+    QualType DestType = Entity.getType();</div><div>+    S.Diag(Kind.getLocation(), diag::err_default_init_const)</div><div>+        << DestType << (bool)DestType->getAs<RecordType>()</div>
<div>+        << FixItHint::CreateInsertion(ZeroInitializationFixitLoc,</div><div>+                                      ZeroInitializationFixit);</div><div>+  }</div><div><br></div></div><div>It might be more natural to emit this diagnostic as part of performing the SK_ZeroInitialization step.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>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.</div>
</div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Or we could omit the fixit and just do the wording change, but the fixit makes the diagnostic much clearer.</div>
</div></blockquote></div></div></div>