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