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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

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.

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 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, or the __declspec based attribute (for pretty printing, as well as parsing, etc).

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];
 }
----------------
Would you mind adding some documentation to the attribute since we're changing it?

================
Comment at: lib/Parse/ParseDecl.cpp:1048
@@ +1047,3 @@
+/// \brief Parse the contents of the "deprecated" attribute.
+unsigned Parser::ParseDeprecatedAttribute(IdentifierInfo &Deprecated,
+                                      SourceLocation DeprecatedLoc,
----------------
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.

================
Comment at: lib/Parse/ParseDecl.cpp:1068
@@ +1067,3 @@
+        if (Keyword != Ident_replacement) {
+        }
+
----------------
I assume this was meant to do something useful?

================
Comment at: lib/Parse/ParseDecl.cpp:1070
@@ +1069,3 @@
+
+        if (Tok.isNot(tok::equal)) {
+          Diag(Tok, diag::err_expected_after) << Keyword << tok::equal;
----------------
This now makes the attribute behave differently than `__attribute__((deprecated))` because you can have this odd named-argument behavior. I would strongly prefer this be an actual optional parameter instead of using this approach. e.g., `__attribute__((deprecated("", "fixit")))`.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5074
@@ +5073,3 @@
+  StringRef Str, Replacement;
+  if ((Attr.isArgExpr(0) && Attr.getArgAsExpr(0) &&
+       !S.checkStringLiteralArgumentAttr(Attr, 0, Str))||
----------------
This check should be moved above the extension warning above -- we only want to diagnose the extension if the attribute is actually applied.

================
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")]];
----------------
This is a bug; we should not be hijacking the gnu attribute space.

================
Comment at: test/SemaCXX/cxx11-attr-print.cpp:18
@@ -17,2 +17,3 @@
 int b [[gnu::deprecated("warning")]];
 
+// CHECK: __attribute__((deprecated("", "")));
----------------
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)`.


http://reviews.llvm.org/D17865