[PATCH] D18997: [SemaObjC] Properly diagnose type arguments and protocols mix in parameterized classes

[Bruno Cardoso Lopes via cfe-commits]

bruno added a comment.

Hi Manman,


================
Comment at: lib/Parse/ParseObjc.cpp:1697
@@ -1696,3 +1696,3 @@
   // We syntactically matched a type argument, so commit to parsing
-  // type arguments.
+  // type arguments. Finding protocol arguments after here is an error.
 
----------------
manmanren wrote:
> I am a little confused about the original comment. Have we syntactically matched a type argument? All we have done is trying to parse the single identifiers, right?
Yes, you're right! Going to update the entire comment to be more accurate

================
Comment at: lib/Parse/ParseObjc.cpp:1720
@@ -1714,3 +1719,3 @@
       if (fullTypeArg.isUsable())
         typeArgs.push_back(fullTypeArg.get());
       else
----------------
manmanren wrote:
> Should we set foundValidTypeId here too?
See the comment below

================
Comment at: lib/Parse/ParseObjc.cpp:1722
@@ -1716,3 +1721,3 @@
       else
         invalid = true;
     } else {
----------------
manmanren wrote:
> Should we emit diagnostics for all cases we set invalid to true?
The initial version of the patch was doing this. However, I could not come up with a testcase to exercise it, so I left it out. Gonna try harder!

================
Comment at: lib/Parse/ParseObjc.cpp:1726
@@ +1725,3 @@
+      auto *P = Actions.LookupProtocol(identifiers[i], identifierLocs[i]);
+      if (!P) {
+        unknownTypeArgs.push_back(identifiers[i]);
----------------
manmanren wrote:
> Please add comments here: not a type argument, not a protocol, treat it as unknown type.
Ok

================
Comment at: lib/Parse/ParseObjc.cpp:1729
@@ +1728,3 @@
+        unknownTypeArgsLoc.push_back(identifierLocs[i]);
+        continue;
+      }
----------------
manmanren wrote:
> Is it necessary to have a "continue" here? Can we restructure this to get rid of the "continue"?
Did this to remove some level of nested "if"s which I personally dislike, but can easily change that


http://reviews.llvm.org/D18997