[cfe-commits] [PATCH] Improved __declspec support

[Richard Smith]

On Mon, Jun 18, 2012 at 1:16 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> Here's a revised patch that takes advantage of Sean Hunt's work on
> attributes as well (and incorporates the changes Richard suggested
> previously).

Some more small things, then please commit :)

> @@ -57,7 +57,10 @@
>    enum Syntax {
>      AS_GNU,
>      AS_CXX11,
> -    AS_Declspec
> +    AS_Declspec,
> +    // eg) __w64, __ptr32, etc.  It is implied that an MSTypespec is also
> +    // a declspec.
>  +    AS_MSTypespec

No need to change this now, but I wonder whether we will want to
generalize this to AS_Keyword.

> +  /// 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). For instance, this change (from later in
the patch) would produce the wrong diagnostic for them (if it were
reachable):

> +      S.Diag(Attr.getLoc(), Attr.isDeclspecAttribute() ?
> +             diag::warn_unhandled_ms_attribute_ignored :
> +             diag::warn_unknown_attribute_ignored) << Attr.getName();


> +  } 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)

> +  } else {
> +
> +    // Allow parsing to continue
> +    return false;

You can deal with this with the same logic: if there's a '(' next,
skip all tokens up to and including the corresponding ')'. In fact,
perhaps just remove the special case for "property" entirely, so we
also produce the warn_ms_declspec_unknown diagnostic. This'd allow us
to deal with
  __declspec(some_unknown_attribute(...) some_known_attribute)

> --- lib/Sema/SemaDeclAttr.cpp	(revision 158672)
> +++ lib/Sema/SemaDeclAttr.cpp	(working copy)
[...]
> +    D->addAttr(::new (S.Context) AlignedAttr(Attr.getRange(), S.Context,
> +                true, 0, Attr.isDeclspecAttribute()));

This (and the other calls you're changing in this file) have an extra
space in the indentation of the second line.