[PATCH] D17865: Add replacement = "xxx" to DeprecatedAttr.

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In http://reviews.llvm.org/D17865#370912, @manmanren wrote:

> Hi Aaron,
>
> Thanks for the review!
>
> > I like the idea of a potential fixit being provided by the attribute, but I do not agree with the way the feature is surfaced in this patch.
>
> > 
>
> > For the GNU-style attribute, the named argument functionality is sufficiently novel syntax that I would strongly like to avoid it, especially since __attribute__((deprecated)) is supported by multiple compilers.
>
>
> I suggested `replacement = ""` mostly for its explicitness, especially when we already have an optional message argument "".
>  If a user only want to specify replacement without a message, we can say deprecated(replacement = "xxx")
>  With 'replacement =`, the user will need to specify an empty message.


I do like the explicit nature of this approach, but I'm worried about it being too novel. For instance, would this compel GCC to implement a similar parsing feature since they also support the deprecated attribute, that sort of thing. Also, it feels like a bit of an odd design in C and C++ since nothing else in the language supports argument passing by name like that (and I would really hate to see us use = blah only to find out that C++ someday standardizes on something different).

> But your concern makes sense too. We can support the following:

>  deprecated("xxx") --> a message

>  deprecated("xxx, "fixit") --> a message and a fixit


I think this is the correct approach, even with multiple optional arguments.

> > We should not be adding this functionality to the C++-style attribute in the gnu namespace; that's not our vendor namespace to modify.

> 

> 

> I believe the patch currently does not change the C++-style attribute. Of course, I should add testing cases to verify.


I believe the patch does change the C++-style attribute in the gnu namespace, as best I can tell. e.g., it would now allow: ``[[gnu::deprecated("something", "a fixit")]]``. We need to be careful not to allow this unless GCC allows the same syntax, otherwise we are hijacking their vendor attribute namespace with something incompatible.

>   I wouldn't be opposed to adding a clang namespace where we support this feature, however.

> 

> > We definitely need to make sure we are not modifying the functionality for the C++ standards-based attribute

> 

> 

> Is this ("the C++ standards-based attribute") the same as "the C++-style attribute" you mentioned above?


This is talking about the C++14 [[deprecated]] attribute. We want to make sure we don't allow [[deprecated("foo", "fixit")]], which I don't think your patch does (but we should have tests ensuring it).

> , or the __declspec based attribute (for pretty printing, as well as parsing, etc).

>  Yes, I should add testing cases to make sure __declspec is not changed at all with this patch.

> 

> > So this change is missing a lot of tests to make sure we do not support this feature in places where it doesn't belong.

> 





http://reviews.llvm.org/D17865