manmanren added a comment.
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.
But your concern makes sense too. We can support the following:
deprecated("xxx") --> a message
deprecated("xxx, "fixit") --> a message and a fixit
> 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 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?
, 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.
================
Comment at: include/clang/Basic/Attr.td:716
@@ -715,2 +715,3 @@
+ StringArgument<"Replacement", 1>];
let Documentation = [Undocumented];
}
----------------
aaron.ballman wrote:
> Would you mind adding some documentation to the attribute since we're
> changing it?
Yes, I should.
================
Comment at: lib/Parse/ParseDecl.cpp:1048
@@ +1047,3 @@
+/// \brief Parse the contents of the "deprecated" attribute.
+unsigned Parser::ParseDeprecatedAttribute(IdentifierInfo &Deprecated,
+ SourceLocation DeprecatedLoc,
----------------
aaron.ballman wrote:
> This also gets called for attributes parsed in the gnu:: namespace, which
> means that you are modifying the behavior of gnu::deprecated(), which we
> should not do unless GCC also supports this feature.
Yes, I will fix this.
================
Comment at: lib/Parse/ParseDecl.cpp:1068
@@ +1067,3 @@
+ if (Keyword != Ident_replacement) {
+ }
+
----------------
aaron.ballman wrote:
> I assume this was meant to do something useful?
Yes, I should emit a diagnostic.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5074
@@ +5073,3 @@
+ StringRef Str, Replacement;
+ if ((Attr.isArgExpr(0) && Attr.getArgAsExpr(0) &&
+ !S.checkStringLiteralArgumentAttr(Attr, 0, Str))||
----------------
aaron.ballman wrote:
> This check should be moved above the extension warning above -- we only want
> to diagnose the extension if the attribute is actually applied.
Agree.
================
Comment at: test/SemaCXX/cxx11-attr-print.cpp:16
@@ -15,3 +15,3 @@
-// CHECK: int b {{\[}}[gnu::deprecated("warning")]];
+// CHECK: int b {{\[}}[gnu::deprecated("warning", "")]];
int b [[gnu::deprecated("warning")]];
----------------
aaron.ballman wrote:
> This is a bug; we should not be hijacking the gnu attribute space.
Yes, I will fix this.
================
Comment at: test/SemaCXX/cxx11-attr-print.cpp:18
@@ -17,2 +17,3 @@
int b [[gnu::deprecated("warning")]];
+// CHECK: __attribute__((deprecated("", "")));
----------------
aaron.ballman wrote:
> I would like a test that verifies we don't print an empty fixit literal when
> using the C++14 `[[deprecated]]` attribute. Same for `__declspec(deprecated)`.
Will do.
http://reviews.llvm.org/D17865
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
