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