[PATCH] D67935: Add `#pragma clang deprecated`, used to deprecate macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D67935#2251145 <https://reviews.llvm.org/D67935#2251145>, @erik.pilkington wrote:

> @aaron.ballman: Did you happen to get any feedback on macro attributes? There are a growing number of macros that we'd like to be able to deprecate, and having a workable solution would be useful for us.

Thank you for bringing this back up! I've worked on a patch to add preprocessor attributes to clang but have set it aside because it feels like it may be an awkward fit because of attribute arguments -- for instance, the preprocessor has no type system or AST, so we track values for things with `APValue` objects and there is no string `APValue` type, so it would be a fair amount of work to support `# [[deprecated("don't use baz")]] define BAZ`, let alone the more esoteric situations for arbitrary attributes. Another issue is that the preprocessor is sometimes shared between C/C++ frontends and, say, a FORTRAN frontend, which could suddenly introduce a new feature into a FORTRAN compiler without extra work.

Based on all of that, I think we should go with your approach of using a `#pragma` as it does solve a problem and isn't quite as novel. However, I am wondering about the design a bit -- why are you using a string literal to supply the macro name? It's my understanding that all preprocessing directives are executed at the same phase of translation (so you don't have to worry about `#define` changing the behavior of `#pragma` or `_Pragma`), so I would have expected a design more like:

  #define FOOBAR(x) whatever(x)
  #pragma clang deprecated(FOOBAR, "don't use FOOBAR, it will do bad things")

This feels like a more approachable way to expose the feature, to me, but I wonder if I'm missing something.

Other feedback is: should we diagnose if the pp-token for the macro identifier doesn't actually match the name of a macro? Or are we allowing constructs like:

  #pragma clang deprecated(FOOBAR, "don't use FOOBAR, twelve is a terrible number")
  #define FOOBAR 12



================
Comment at: clang/lib/Lex/Pragma.cpp:1544
+    PP.Lex(Tok);
+    if (!PP.FinishLexStringLiteral(Tok, EntityString,
+                                   "#pragma clang deprecated",
----------------
Do we want to allow arbitrary string literals, or only narrow character string literals? e.g., should we disallow something like: `#pragma clang deprecated(L"oops", U"hahahaha")` ?


================
Comment at: clang/test/Lexer/pragma-deprecated.c:17-20
+#pragma clang deprecated("\"flerp.h\"", "use \"flarp.h\" instead")
+
+// expected-warning at +1{{'flerp' is deprecated}}
+#pragma clang deprecated("flerp")
----------------
What is the utility of unconditionally emitting a diagnostic like this? We've already got `#warning`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67935