[PATCH] D25816: Use descriptive message if list initializer is incorrectly parenthesized.

[Alex Lorenz via cfe-commits]

arphaman added a comment.

Thanks for working on this! I have a couple of comments:



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1762
 def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
+def err_list_init_in_parens : Error<"list-initializer for non-class type "
+  "must not be parenthesized">;
----------------
Wouldn't it be better if we had a diagnostic with the type information? So, instead of showing `list-initializer for non-class type must not be parenthesized` clang would show `list-initializer for non-class type 'int' must not be parenthesized`. What do you think?

As well as that, it seems that GCC issues a warning instead of an error for this diagnostic, so should this be a warning in clang as well?


================
Comment at: include/clang/Sema/Sema.h:1802
   void ActOnInitializerError(Decl *Dcl);
+  bool cannotBeInitializededByParenthzedList(QualType TargetType);
   void ActOnPureSpecifier(Decl *D, SourceLocation PureSpecLoc);
----------------
I think you mean `parenthesized` instead of `parenthzed` here. As well as that, I think that it would be better to use a name without the `not`, something like `canInitializeWithParenthesizedList`.

Also, please add a comment that explains what this method does.


================
Comment at: include/clang/Sema/Sema.h:1803
+  bool cannotBeInitializededByParenthzedList(QualType TargetType);
   void ActOnPureSpecifier(Decl *D, SourceLocation PureSpecLoc);
   void ActOnCXXForRangeDecl(Decl *D);
----------------
Please add a blank line here to separate unrelated `Act...` methods from the new method.


================
Comment at: lib/Sema/SemaDecl.cpp:9796
+        Diag(VDecl->getLocation(), diag::err_list_init_in_parens)
+          << CXXDirectInit->getSourceRange()
+          << FixItHint::CreateRemoval(CXXDirectInit->getLocStart())
----------------
Slight formatting issue: please indent the three lines that start with '<<' using 4 extra spaces instead of 2  (just like you did in your changes for SemaExprCXX.cpp).


================
Comment at: lib/Sema/SemaDecl.cpp:10110
+bool Sema::cannotBeInitializededByParenthzedList(QualType TargetType) {
+  return !TargetType->isDependentType() && !TargetType->isRecordType() &&
+         !TargetType->getContainedAutoType();
----------------
If you change the name as I suggested to something like `canInitializeWithParenthesizedList` then the logic here would have to be inverted: `TargetType()->isDependentType() || TargetType()->isRecordType() || TargetType()->getContainedAutoType()`. Consequently, the call sites will have to be updated to include a `!` before the call.


================
Comment at: lib/Sema/SemaExprCXX.cpp:1229
+      Diag(TInfo->getTypeLoc().getLocStart(), diag::err_list_init_in_parens)
+          << IList->getSourceRange()
+          << FixItHint::CreateRemoval(LParenLoc)
----------------
Formatting issue: clang-format replaces

```
          << IList->getSourceRange()
          << FixItHint::CreateRemoval(LParenLoc)
```
with
```
          << IList->getSourceRange() << FixItHint::CreateRemoval(LParenLoc)
```


================
Comment at: test/SemaCXX/cxx0x-initializer-scalars.cpp:96
+    (void) int({0}); // expected-error {{list-initializer for non-class type must not be parenthesized}}
+    new int({0});  // expected-error {{list-initializer for non-class type must not be parenthesized}}
   }
----------------
Please add a test case for a pointer type as well, something like this should work:
```
int *x({0});
```
As well as that, please add a test for a dependent type.


https://reviews.llvm.org/D25816