[PATCH, RFC] Fix PR16456 (make bool a context-sensitive keyword in Altivec mode)

[Richard Smith]

On Wed, Jul 3, 2013 at 1:43 PM, Bill Schmidt
<wschmidt at linux.vnet.ibm.com> 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
>> > <wschmidt at linux.vnet.ibm.com> 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.