[PATCH] D106732: Support macro deprecation #pragma clang deprecated

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I think it's really useful functionality!



================
Comment at: clang/docs/LanguageExtensions.rst:3875
+   #define MIN(x, y) x < y ? x : y
+   #pragma clang deprecated("MIN", "use std::min instead")
+
----------------
Rather than use a string literal, did you consider using an unexpanded identifier token?


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:523-524
+// - #pragma deprecate(...)
+def err_pragma_deprecated_expected :
+  Error<"expected '%0' in #pragma clang deprecate">;
+def warn_pragma_deprecated_not_a_macro :
----------------
We have an existing diagnostic for this -- `diag::err_expected`.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:526
+def warn_pragma_deprecated_not_a_macro :
+  ExtWarn<"'%0' is not a defined macro for #pragma clang deprecate">,
+  InGroup<DeprecatedPragma>;
----------------
I think we should consider reusing (and renaming) `err_pp_visibility_non_macro` for this purpose. I think it should be an error  to try to deprecate a macro that doesn't exist because that feels like a really confusing situation for programmers (what does it mean?), but I don't feel strongly. However, I don't think this is an `ExtWarn` situation (pragmas are a bit weird; when I took a look, we're wildly inconsistent here) because we wouldn't issue a "blah is a Clang extension" warning about it.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:528-533
+def warn_pragma_deprecated_macro_use :
+  ExtWarn<"macro '%0' has been marked as deprecated">,
+  InGroup<DeprecatedPragma>;
+def warn_pragma_deprecated_macro_use_msg :
+  ExtWarn<"macro '%0' has been marked as deprecated: %1">,
+  InGroup<DeprecatedPragma>;
----------------
Can you get away with something along these lines so we only use a single diagnostic message?


================
Comment at: clang/include/clang/Basic/IdentifierTable.h:127
+
+  // 24 bits left in a 64-bit word.
 
----------------
I come up with the same number as you did. Thank you for updating this!


================
Comment at: clang/include/clang/Lex/Preprocessor.h:2391
 
+  void addMacroDeprecationMsg(IdentifierInfo *II, std::string Msg) {
+    MacroDeprecationMsgs.insert(std::make_pair(II, Msg));
----------------



================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:478
+      Diag(Identifier, diag::warn_pragma_deprecated_macro_use)
+          << Identifier.getIdentifierInfo()->getName();
+    else
----------------



================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:481
+      Diag(Identifier, diag::warn_pragma_deprecated_macro_use_msg)
+          << Identifier.getIdentifierInfo()->getName() << MsgEntry->second;
+  }
----------------



================
Comment at: clang/lib/Lex/Pragma.cpp:1914
 
+/// "\#pragma deprecate()"
+///
----------------



================
Comment at: clang/lib/Lex/Pragma.cpp:1918
+/// \code
+///   #pragma clang deprecate(MACRO_NAME)
+/// \endcode
----------------
The syntax looks off here, it doesn't mention the message.


================
Comment at: clang/lib/Lex/Pragma.cpp:1939
+
+    if (!II->hasMacroDefinition() || !II->hadMacroDefinition()) {
+      PP.Diag(Tok, diag::warn_pragma_deprecated_not_a_macro) << II->getName();
----------------
Can you explain why you're looking for a macro that's been undefined? I'm wondering what the expectations are for code like:
```
#define FOO 12
#undef FOO
#pragma clang deprecated("FOO")
```
The user can't expand `FOO`, so there's nothing there to deprecate. But if a user later does `#define FOO 42`, it's not clear to me that we *should* diagnose use of `FOO` as being deprecated at that point. Once the macro is undefined, that signals at least some intent "I'm done with this identifier as a macro" and so the deprecation seems like it should perhaps not hold?


================
Comment at: clang/test/Lexer/deprecate-macro.c:1
+// RUN: not %clang_cc1 -Wdeprecated %s -fsyntax-only
+
----------------
Without the -verify, this doesn't test the expected diagnostics. Also, I don't think you need `not` there once you're verifying the results.


================
Comment at: clang/test/Lexer/deprecate-macro.c:35
+#endif
+
+int main(int argc, char** argv) {
----------------
Some other useful test cases:
```
#define frobble 1
#pragma clang deprecated("frobble")

#undef frobble // Expect no diagnostics here

#define bobble 1
#pragma clang deprecated("bobble")

#define bobble 1 // Do we expect a diagnostic here?
#define frobble 1 // How about here given that this was undefined?
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106732/new/

https://reviews.llvm.org/D106732