On Wed, Jul 3, 2013 at 1:43 PM, Bill Schmidt <[email protected]> wrote: > On Wed, 2013-07-03 at 15:16 -0500, Bill Schmidt wrote: >> On Wed, 2013-07-03 at 13:08 -0700, Richard Smith wrote: >> > On Wed, Jul 3, 2013 at 12:32 PM, Bill Schmidt >> > <[email protected]> wrote: >> > > "bool" should be a context-sensitive keyword in Altivec mode. >> > > >> > > PR16456 reported that Clang implements a hybrid between AltiVec's >> > > "Keyword and Predefine Method" and its "Context Sensitive Keyword >> > > Method," where "bool" is always a keyword, but "vector" and "pixel" >> > > are context-sensitive keywords. This isn't permitted by the AltiVec >> > > spec. For consistency with gcc, this patch implements the Context >> > > Sensitive Keyword Method for bool, and stops treating true and false >> > > as keywords in Altivec mode. >> > > >> > > The patch removes KEYALTIVEC as a trigger for defining these keywords >> > > in include/clang/Basic/TokenKinds.def, and adds logic for "vector >> > > bool" that mirrors the existing logic for "vector pixel." The test >> > > case is taken from the bug report. >> > > >> > > I know very little about the front end, so I would appreciate some >> > > review prior to committing the patch. Thanks! >> > >> > The patch contains windows line endings. Please make sure you don't >> > commit with windows line endings. >> >> That's...odd. Evolution must have messed it up. I work only in Linux. >> >> > >> > --- include/clang/Parse/Parser.h (revision 185433) >> > +++ include/clang/Parse/Parser.h (working copy) >> > @@ -531,7 +532,8 @@ class Parser : public CodeCompletionHandler { >> > bool &isInvalid) { >> > if (!getLangOpts().AltiVec || >> > (Tok.getIdentifierInfo() != Ident_vector && >> > - Tok.getIdentifierInfo() != Ident_pixel)) >> > + Tok.getIdentifierInfo() != Ident_pixel && >> > + Tok.getIdentifierInfo() != Ident_bool)) >> > >> > This looks wrong: >> > >> > bool x; >> > >> > is not valid with -faltivec; bool is only a keyword after 'vector'. We >> > shouldn't be handling 'pixel' here either, FWIW. Per the AltiVec spec, >> > 2.2.2: >> > >> > "Since vector must be first among the type specifiers, it can be >> > recognized as a type >> > specifier when a type identifier is being scanned. The new uses of pixel >> > and bool occur >> > after vector has been recognized. In all other contexts, vector, >> > pixel, and bool are not >> > reserved." >> >> OK, I'll remove handling for both and re-test. >> >> > >> > >> > --- lib/Parse/ParseDecl.cpp (revision 185433) >> > +++ lib/Parse/ParseDecl.cpp (working copy) >> > @@ -5623,6 +5631,10 @@ bool Parser::TryAltiVecTokenOutOfLine(DeclSpec &DS >> > DS.isTypeAltiVecVector()) { >> > isInvalid = DS.SetTypeAltiVecPixel(true, Loc, PrevSpec, DiagID); >> > return true; >> > + } else if ((Tok.getIdentifierInfo() == Ident_bool) && >> > + DS.isTypeAltiVecVector()) { >> > + isInvalid = DS.SetTypeAltiVecBool(true, Loc, PrevSpec, DiagID); >> > + return true; >> > } >> > return false; >> > } >> > >> > Likewise, this looks wrong, as does the handling of 'pixel' before it. >> > >> >> OK, likewise with the removal/re-test. ;) > > Richard, if I remove either of these chunks, clang fails to recognize > "vector bool" and "vector pixel" as legitimate constructs. > > Regarding the second chunk, I believe the test DS.isTypeAltiVecVector() > indicates that we've already recognized that we're inside a "vector" > decl when processing "pixel" or "bool". And without the first chunk we > never get to check things for the second.
Oh, I see, the code is recognising the 'vector' and the following keyword together but then handling them independently. That's weird, but in that light, your original patch looks fine. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
