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

[Manman Ren via cfe-commits]

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