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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
