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