<div class="gmail_quote">On Wed, Jun 27, 2012 at 2:10 PM, Nikola Smiljanic <span dir="ltr"><<a href="mailto:popizdeh@gmail.com" target="_blank">popizdeh@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The patch adds a note and a fixit for the missing case of vexing parse<br>
(it's even covered here with clang output<br>
<a href="http://en.wikipedia.org/wiki/Most_vexing_parse" target="_blank">http://en.wikipedia.org/wiki/Most_vexing_parse</a>).<br>
fixit-vexing-parse.cpp and fixit-vexing-parse-cxx0x.cpp contain one<br>
additional test each, since the wording is different for c++11. Other<br>
changes are only fixes for failing tests.<br></blockquote><div><br></div><div>Very cool! The patch mostly looks good, but I have a few comments...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

The text for note_replace_parens_for_variable_declaration is copied<br>
from DiagnosticSemaKinds.td verbatim. I couldn't use the existing note<br>
from Sema since Parse code doesn't know about it.<br>
</blockquote></div><br><div>The wording for that note isn't appropriate for this situation: it should be 'replace parentheses with braces' rather than 'replace parentheses with an initializer'. But I think you should be more careful before deciding to produce this note; it's wrong in cases like these:</div>
<div><div><br></div><div>  std::vector<std::string> v1(std::istream_iterator<std::string>(cin), std::istream_iterator<std::string>());</div><div>  std::vector<T> v2(size_t(getNumElements()), T());</div>
</div><div><br></div><div>Your FixIt will cause these declarations to use vector's initializer_list constructor, which is presumably not what the author of the code intended. We can't detect these cases from within the parser, so this check should either always use the C++98 note, or (preferably, but more work) be moved to Sema (alongside the "int n()" vexing parse diagnostic code) and use the braces note only if there is no initializer_list constructor. Moving this check to Sema would also allow us to easily suppress the bogus warning on code like:</div>
<div><br></div><div>  typedef void V;</div><div>  void f() {</div><div>    V g(int (*p)[4]);</div><div>  }</div><div><br></div><div>... on the basis that the return type is 'void', in the same way we suppress it for V g();</div>
<div><br></div><div>This change will also produce an unhelpful note for this case (in C++98 mode)...</div><div><br></div><div>  if (int n(int())) {</div><div><br></div><div>... because a direct-initializer isn't actually permitted on this declaration, but we perform a tentative parse anyway in order to produce the 'variable declaration in condition cannot have a parenthesized initializer' diagnostic. For now, I suggest you just set warnIfAmbiguous to false in ParseDirectDeclarator when we're in a Declarator::ConditionContext.</div>
<div><br></div><div><div>+      else</div><div>+        Diag(Tok, diag::note_additional_parens_for_variable_declaration)</div><div>+          << FixItHint::CreateInsertion(Tok.getLocation(), "(")</div><div>
+          << FixItHint::CreateInsertion(TPLoc, ")");</div></div><div><br></div><div>This fixit isn't correct if multiple parameters are declared: you're suggesting adding a single set of parentheses around all the parameters; instead, you should suggest adding them around just one of the parameters.</div>