[cfe-commits] Note and FixIt for additional case of vexing parse

[Richard Smith]

On Wed, Jun 27, 2012 at 2:10 PM, Nikola Smiljanic <popizdeh at gmail.com>wrote:

> The patch adds a note and a fixit for the missing case of vexing parse
> (it's even covered here with clang output
> http://en.wikipedia.org/wiki/Most_vexing_parse).
> fixit-vexing-parse.cpp and fixit-vexing-parse-cxx0x.cpp contain one
> additional test each, since the wording is different for c++11. Other
> changes are only fixes for failing tests.
>

Very cool! The patch mostly looks good, but I have a few comments...


> The text for note_replace_parens_for_variable_declaration is copied
> from DiagnosticSemaKinds.td verbatim. I couldn't use the existing note
> from Sema since Parse code doesn't know about it.
>

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:

  std::vector<std::string> v1(std::istream_iterator<std::string>(cin),
std::istream_iterator<std::string>());
  std::vector<T> v2(size_t(getNumElements()), T());

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:

  typedef void V;
  void f() {
    V g(int (*p)[4]);
  }

... on the basis that the return type is 'void', in the same way we
suppress it for V g();

This change will also produce an unhelpful note for this case (in C++98
mode)...

  if (int n(int())) {

... 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.

+      else
+        Diag(Tok, diag::note_additional_parens_for_variable_declaration)
+          << FixItHint::CreateInsertion(Tok.getLocation(), "(")
+          << FixItHint::CreateInsertion(TPLoc, ")");

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120627/10a0d703/attachment.html>