Page MenuHomePhabricator

Support macro deprecation #pragma clang deprecated
ClosedPublic

Authored by beanz on Jul 23 2021, 6:03 PM.

Details

Summary

This patch adds #pragma clang deprecated to enable deprecation of
preprocessor macros.

The macro must be defined before #pragma clang deprecated. When
deprecating a macro a custom message may be optionally provided.

Warnings are emitted at the use site of a deprecated macro, and can be
controlled via the -Wdeprecated warning group.

This patch takes some rough inspiration and a few lines of code from
https://reviews.llvm.org/D67935.

Diff Detail

Event Timeline

beanz created this revision.Jul 23 2021, 6:03 PM
beanz requested review of this revision.Jul 23 2021, 6:03 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
beanz updated this revision to Diff 361480.Jul 24 2021, 2:38 PM

Updating comment about remaining bits.

Rather than subtracting one, I re-added up the bits. The bitfield is now at
40 bits, so it only has 24 bits remaining (someone please check my math).

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

clang/docs/LanguageExtensions.rst
3898

Rather than use a string literal, did you consider using an unexpanded identifier token?

clang/include/clang/Basic/DiagnosticLexKinds.td
523–524

We have an existing diagnostic for this -- diag::err_expected.

526

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.

528–533

Can you get away with something along these lines so we only use a single diagnostic message?

clang/include/clang/Basic/IdentifierTable.h
127

I come up with the same number as you did. Thank you for updating this!

clang/include/clang/Lex/Preprocessor.h
2398
clang/lib/Lex/PPMacroExpansion.cpp
483
486
clang/lib/Lex/Pragma.cpp
1914
1918

The syntax looks off here, it doesn't mention the message.

1939

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?

clang/test/Lexer/deprecate-macro.c
2

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.

36

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?
beanz added a comment.Jul 26 2021, 8:50 AM

Will update the patch later today with your feedback.

clang/docs/LanguageExtensions.rst
3898

The Lexer expands preprocessor macros, so you wouldn't get an identifier token, you actually get the tokens inside the macro. I suspect this is why #pragma push_macro does the same thing and uses a string literal.

aaron.ballman added inline comments.Jul 26 2021, 9:14 AM
clang/docs/LanguageExtensions.rst
3898

You can lex in either mode. See LexUnexpandedToken().

beanz updated this revision to Diff 361761.Jul 26 2021, 12:32 PM

Updates based on feedback from @aaron.ballman

beanz updated this revision to Diff 361831.Jul 26 2021, 3:27 PM

Fixing handling of #ifdef, #ifndef, and defined()

All tests pass this time :)

beanz updated this revision to Diff 362028.Jul 27 2021, 7:35 AM
beanz marked 13 inline comments as done.

Minor comment fix

beanz added a comment.Jul 27 2021, 7:36 AM

All feedback should be addressed

beanz updated this revision to Diff 362049.Jul 27 2021, 8:51 AM

Fix warning printing always having a : on the end.

beanz updated this revision to Diff 362052.Jul 27 2021, 8:54 AM

Minor comment fixes... I swear, eventually I'll be done...

beanz updated this revision to Diff 362053.Jul 27 2021, 8:56 AM

Missed one

beanz updated this revision to Diff 362123.Jul 27 2021, 11:51 AM

One last fix for a test failure my change caused.

aaron.ballman added inline comments.Jul 28 2021, 12:01 PM
clang/test/Lexer/deprecate-macro.c
35–40

Some more test cases to add:

// Test that we diagnose on #elifdef.
#ifdef baz
#elifdef foo // expected-warning
#endif

// Test that we diagnose on #elifndef.
#ifdef baz
#elifndef foo // expected-warning
#endif

// Test that we diagnose both the taken branch and the skipped branch.
#ifdef foo // expected-warning
#elifdef bar // expected-warning
#endif
beanz updated this revision to Diff 362528.Jul 28 2021, 1:49 PM

Covered taken #elif* directives per @aaron.ballmon's feedback.

Handling non-taken #elif directives is non-trivial because clang skips parsing the conditionals for non-taken directives. At present clang won't even error on malformed #elif directives if an earlier branch is taken.

Also with this update I refactored the error emitting code out to a function on the Preprocessor, since it is just getting copy and pasted over and over again, and this change would have added another copy.

beanz updated this revision to Diff 362574.Jul 28 2021, 4:01 PM

Moving the warning emission later.

Covered taken #elif* directives per @aaron.ballmon's feedback.

Thanks!

Handling non-taken #elif directives is non-trivial because clang skips parsing the conditionals for non-taken directives. At present clang won't even error on malformed #elif directives if an earlier branch is taken.

Yeah, that's what I was worried about. Can you add some FIXME comments into Preprocessor::SkipExcludedConditionalBlock() about wanting to diagnose this situation, and add some test cases with FIXME comments showing we know we don't handle this case perfectly yet?

Also with this update I refactored the error emitting code out to a function on the Preprocessor, since it is just getting copy and pasted over and over again, and this change would have added another copy.

Thank you, that's a nice cleanup!

clang/include/clang/Lex/Preprocessor.h
2409
clang/lib/Lex/Preprocessor.cpp
1416
clang/test/Lexer/deprecate-macro.c
63
beanz updated this revision to Diff 362764.Jul 29 2021, 7:11 AM
beanz marked 3 inline comments as done.

Addressing @aaron.ballman's feedback.

I've added FIXMEs and test cases with not-expected annotations so that in the future if the underlying issue gets fixed, the tests should only need updating from not-expected to expected.

aaron.ballman accepted this revision.Jul 29 2021, 10:38 AM

LGTM, thank you! I am excited to start using this shortly. :-)

This revision is now accepted and ready to land.Jul 29 2021, 10:38 AM
This revision was automatically updated to reflect the committed changes.