<div dir="ltr">Thank you for the review. I added a test to check the "redeclaration of type parameter" error message and committed it as r<span style="font-size:12.8px">255754.</span></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 15, 2015 at 4:55 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> writes:<br>
> On Tue, Dec 15, 2015 at 12:55 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>><br>
> wrote:<br>
><br>
>> Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> writes:<br>
>> > On Mon, Dec 14, 2015 at 10:39 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>><br>
>> > wrote:<br>
>> ><br>
>> >> Akira Hatanaka via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> writes:<br>
>> >> > ahatanak created this revision.<br>
>> >> > ahatanak added a subscriber: cfe-commits.<br>
>> >> ><br>
>> >> > This patch fixes a crash that occurs when __kindof is incorrectly used<br>
>> >> > in the type parameter list of an interface. The crash occurs because<br>
>> >> > ObjCTypeParamList::back() is called in checkTypeParamListConsistency<br>
>> >> > on an empty list:<br>
>> >> ><br>
>> >> > 00762 diagLoc =<br>
>> >> S.getLocForEndOfToken(newTypeParams->back()->getLocEnd());<br>
>> >> ><br>
>> >> > <a href="http://reviews.llvm.org/D15463" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15463</a><br>
>> >> ><br>
>> >> > Files:<br>
>> >> > lib/Parse/ParseObjc.cpp<br>
>> >> > test/SemaObjC/kindof.m<br>
>> >> ><br>
>> >> > Index: test/SemaObjC/kindof.m<br>
>> >> > ===================================================================<br>
>> >> > --- test/SemaObjC/kindof.m<br>
>> >> > +++ test/SemaObjC/kindof.m<br>
>> >> > @@ -302,3 +302,13 @@<br>
>> >> > void processCopyable(__typeof(getSomeCopyable()) string);<br>
>> >> > processCopyable(0); // expected-warning{{null passed to a callee<br>
>> that<br>
>> >> requires a non-null argument}}<br>
>> >> > }<br>
>> >> > +<br>
>> >> > +// __kinddof cannot be used in parameter list.<br>
>> >> > +@interface Array1<T> : NSObject<br>
>> >> > +@end<br>
>> >> > +<br>
>> >> > +@interface I1 : NSObject<br>
>> >> > +@end<br>
>> >> > +<br>
>> >> > +@interface Array1<__kindof I1*>(extensions) // //<br>
>> >> expected-error{{expected type parameter name}}<br>
>> >> > +@end<br>
>> >> > Index: lib/Parse/ParseObjc.cpp<br>
>> >> > ===================================================================<br>
>> >> > --- lib/Parse/ParseObjc.cpp<br>
>> >> > +++ lib/Parse/ParseObjc.cpp<br>
>> >> > @@ -603,7 +603,7 @@<br>
>> >> > // whether there are any protocol references.<br>
>> >> > lAngleLoc = SourceLocation();<br>
>> >> > rAngleLoc = SourceLocation();<br>
>> >> > - return list;<br>
>> >> > + return invalid ? nullptr : list;<br>
>> >><br>
>> >> This looks a bit suspicious to me. Since `invalid` is set *way* earlier<br>
>> >> in the function, it seems like we should be able to return earlier if<br>
>> >> this is correct, and not even call actOnObjCTypeParamList. OTOH, are<br>
>> >> there cases where `invalid == true` but list is non-empty? If so, are we<br>
>> >> doing the right thing when that happens?<br>
>> >><br>
>> >><br>
>> > I'm assuming you meant it should return earlier if this is *incorrect*<br>
>> (but<br>
>> > let me know if I misunderstood your comment).<br>
>><br>
>> I meant, "If `return invalid ? nullptr : list` is correct, then I<br>
>> suspect returning nullptr before calling actOnObjCTypeParamList makes<br>
>> more sense.<br>
>><br>
>> > The only downside to returning nullptr before actOnObjCTypeParamList is<br>
>> > called in line 595 is that it won't print the diagnostics that are<br>
>> printed<br>
>> > in actOnObjCTypeParamList. For example, if the following interface were<br>
>> > compiled,<br>
>> ><br>
>> > @interface I1<T, T, __kindof I2*> NSObject<br>
>> > @end<br>
>> ><br>
>> > actOnObjCTypeParamList could tell the user that parameter "T" was<br>
>> > redeclared in addition to printing the error message about __kindof. So<br>
>> if<br>
>> > we return before it's called, we won't see that error message.<br>
>> ><br>
>> > When the interface declaration of category extensions1 in the following<br>
>> > piece of code is compiled, invalid is set to true and the list is not<br>
>> empty<br>
>> > (it contains the first two parameters).<br>
>> ><br>
>> > @interface I1<T1, T2, T3> NSObject<br>
>> > @end<br>
>> ><br>
>> > @interface I1<T, T, __kindof I2*> (extensions1)<br>
>> > @end<br>
>> ><br>
>> > *test3.m:6:23: **error: **expected type parameter name*<br>
>> ><br>
>> > @interface I6<T1, T2, __kindof I2*> (extensions1)<br>
>> ><br>
>> > * ^*<br>
>> ><br>
>> > *test3.m:6:21: **error: **category has too few type parameters (expected<br>
>> 3,<br>
>> > have 2)*<br>
>> ><br>
>> > @interface I6<T1, T2, __kindof I2*> (extensions1)<br>
>> > It looks like it's doing the right thing, but the second diagnostic<br>
>> doesn't<br>
>> > seem necessary to me.<br>
>><br>
>> I'm not sure I follow exactly which errors are being reported with your<br>
>> patch, so maybe this is already working, but what we basically want is<br>
>> to only emit errors that are helpful.<br>
>><br>
>> That is, if we get the `"T" is a redeclaration` error as well as the<br>
>> __kindof error, that's nice, but if we get the `too few type parameters`<br>
>> error that's going to be confusing and bad - the user didn't provide too<br>
>> few type parameters, but our previous error caused us to see too few.<br>
>><br>
>> Basically, we don't ever want to report errors that are the result of<br>
>> previous errors, but as long as we don't end up doing that then<br>
>> gathering further errors is totally reasonable. Make sense?<br>
>><br>
>><br>
> Yes, that makes perfect sense.<br>
<br>
</div></div>Thanks for the detailed explanation! This LGTM then.<br>
<div class="HOEnZb"><div class="h5"><br>
> The following shows the error messages I see when I compile test case<br>
> test3.m:<br>
><br>
> $ cat test3.m<br>
><br>
> #import <Foundation/Foundation.h><br>
> @interface I2 : NSObject<br>
> @end<br>
><br>
> @interface I7<T0, T1, T2> : NSObject<br>
> @end<br>
><br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> @end<br>
><br>
> @interface I6<T> : NSObject<br>
> @end<br>
><br>
> @interface I6<__kindof I2*>(ext1)<br>
> @end<br>
><br>
><br>
> ### This patch: it prints the error messages we want to see.<br>
> test3.m:9:21: error: expected type parameter name<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ^<br>
><br>
> test3.m:9:18: error: redeclaration of type parameter 'T'<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ~ ^<br>
><br>
> test3.m:15:15: error: expected type parameter name<br>
> @interface I6<__kindof I2*>(ext1)<br>
> ^<br>
><br>
> 3 errors generated.<br>
><br>
> ### If it returns nullptr before actOnObjCTypeParamList is called: it<br>
> doesn't print the "redeclaration of type parameter" error.<br>
><br>
> test3.m:9:21: error: expected type parameter name<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ^<br>
><br>
> test3.m:15:15: error: expected type parameter name<br>
> @interface I6<__kindof I2*>(ext1)<br>
> ^<br>
><br>
> 2 errors generated.<br>
><br>
> ### Trunk: It prints the "too few type parameters" message and it crashes.<br>
><br>
> test3.m:9:21: error: expected type parameter name<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ^<br>
><br>
> test3.m:9:18: error: redeclaration of type parameter 'T'<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ~ ^<br>
><br>
> test3.m:9:19: error: category has too few type parameters (expected 3, have<br>
> 2)<br>
> @interface I7<T, T, __kindof I2*>(ext2)<br>
> ^<br>
><br>
> test3.m:15:15: error: expected type parameter name<br>
> @interface I6<__kindof I2*>(ext1)<br>
> ^<br>
><br>
> clang: error: unable to execute command: Segmentation fault: 11<br>
><br>
>><br>
>> >> }<br>
>> >> ><br>
>> >> > /// Parse an objc-type-parameter-list.<br>
>> >> ><br>
>> >> ><br>
>> >><br>
>><br>
</div></div></blockquote></div><br></div>