[cfe-commits] [PATCH] Improved __declspec support

[Aaron Ballman]

On Mon, Jun 18, 2012 at 6:17 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> +  /// Returns true if the attribute is a pure __declspec or a synthesized
>> +  /// declspec representing a type specification (like __w64 or __ptr32).
>> +  bool isDeclspecAttribute() const { return SyntaxUsed == AS_Declspec ||
>> +                                            SyntaxUsed == AS_MSTypespec; }
>
> Given that these attributes aren't spelled as __declspec, I'd prefer
> that we return false for MSTypespec attributes here (and change the
> comment on AS_MSTypespec above to match). I think this makes things
> simpler, and doesn't seem to matter for the rest of the patch (you
> remove the one existing use of this function, and none of your added
> calls need this behavior).

This one kind of bugged me too, but the reason I did it was for
historical purposes.  Those have always been considered __declspec
previously, so this patch was retaining compatibility with the
previous behavior.

I think I would prefer to make that change as a separate patch just to
keep the distinction more clear.  I do agree that it would be better
for those type specifiers to not be spelled as declspecs, but I have
no idea the scope of that sort of change.

>> +  } else if (Ident->getName() == "property") {
> [...]
>> +    return true;
>
> How about recovering from this, with something like:
>
>  BalancedDelimiterTracker T(*this, tok::l_paren);
>  if (!T.consumeOpen())
>    T.skipToEnd();
>  return;
>
> ... then remove the return value from the function. This would allow
> us to deal with things like
>  __declspec(property(...) some_other_attribute)

My next patch (already mostly written) adds true parsing support for
property, so that's a WIP.  But until that patch lands, this is a good
idea.

~Aaron