Page MenuHomePhabricator

Implement #pragma clang final extension
ClosedPublic

Authored by beanz on Aug 23 2021, 11:02 AM.

Details

Summary

This patch adds a new preprocessor extension `#pragma clang final`
which enables warning on undefinition and re-definition of macros.

The intent of this warning is to extend beyond `-Wmacro-redefined` to
warn against any and all alterations to macros that are marked final.

This warning is part of the `-Wpedantic-macros` diagnostics group.

Diff Detail

Event Timeline

beanz requested review of this revision.Aug 23 2021, 11:02 AM
beanz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2021, 11:02 AM
aaron.ballman added inline comments.Sep 8 2021, 8:35 AM
clang/docs/LanguageExtensions.rst
3968–3984

Design question: would it make sense to extend this slightly so that the macro does not have to be defined in order to be finalized? e.g., this could be used as a way for a library author to say "this identifier cannot be defined as a macro"?

3979

What happens if the redefinition is to the same token sequence as the original definition? e.g.,

#define FINAL_MACRO 1+1
#pragma clang final(FINAL_MACRO)
#define FINAL_MACRO 1+1 // ok?
#define FINAL_MACRO (1+1) // Whoa...slow your roll there, champ!
clang/include/clang/Basic/DiagnosticLexKinds.td
544

Heh, I like your approach, but a goal of %select is to ease translation of our diagnostics to other languages (in theory, anyway).

clang/include/clang/Basic/IdentifierTable.h
196
clang/include/clang/Lex/Preprocessor.h
828
clang/lib/Lex/Pragma.cpp
2083

This should cause the macro name to be properly quoted in the diagnostic.

beanz added inline comments.Sep 15 2021, 8:14 PM
clang/docs/LanguageExtensions.rst
3968–3984

That's an interesting thought. I can look into that. Since the current implementation relies on the identifier info I'm not sure how much work it would be to support that.

3979

-Wmacro-redefined currently warns on redefinitions even if they are the same as the existing definition.

The implementation in this patch only triggers on redefining macros that have been undef'd and relies on -Wmacro-redefined to catch masking redefinitions. Although I should probably change that so that final catches on both.

clang/include/clang/Basic/DiagnosticLexKinds.td
544

Ha... Okay... Can do :)

beanz updated this revision to Diff 372862.Sep 15 2021, 9:03 PM

Updates based on feedback from @aaron.ballman.

aaron.ballman added inline comments.Sep 22 2021, 5:04 AM
clang/docs/LanguageExtensions.rst
3979

-Wmacro-redefined currently warns on redefinitions even if they are the same as the existing definition.

Okay, SGTM.

The implementation in this patch only triggers on redefining macros that have been undef'd and relies on -Wmacro-redefined to catch masking redefinitions. Although I should probably change that so that final catches on both.

+1

clang/test/Lexer/final-macro.c
6–9

99% sure I got the syntax right, but you can specify a number to avoid duplicating the diagnostic multiple times and I'm pretty sure it works with the @+N syntax as well, but I don't recall trying in recent history.

11–12

If the goal is to test that mention of the macro here does not cause diagnostics, I'd recommend adding this to the end of the file instead of the beginning -- from there it's more obvious that none of the preceding diagnostics are triggered because of those pragmas.

15

This previous definition marker looks wrong to me -- it should be pointing to line 4, right?

19

Should we suppress this diagnostic when we know we're already issuing the previous one? I get why they both are issued, but it does seem a bit unclean to have two warnings that are basically both "you are redefining this macro and maybe you should reconsider that" diagnostics. (I don't feel strongly, mostly wondering out loud.)

beanz marked 3 inline comments as done and an inline comment as not done.Sep 22 2021, 2:56 PM

Will push updates in a second.

clang/test/Lexer/final-macro.c
15

Since the macro is redefined here, when the later warning gets hit the note pops up here, which makes sense. This itself being a redefinition is amusing, but I think this is probably the way we want it to work. I don't have strong opinions though...

19

I can see this going both ways. I tend to err toward listing _all_ the warnings in case someone wants to try and suppress them for a specific use case. Otherwise they have to build multiple times in a cycle to suppress everything.

beanz updated this revision to Diff 374377.Sep 22 2021, 2:57 PM

Updates based on feedback from @aaron.ballman.

aaron.ballman accepted this revision.Sep 23 2021, 10:40 AM

This LGTM, but please wait a bit before landing in case @rsmith has concerns.

clang/test/Lexer/final-macro.c
15

Ohhhh! I see it now. There were two diagnostics for Foo and this is the note for the most recent previous definition. Got it, thanks!

19

I think the current behavior is defensible; if a user has a real world problem with it, we can address it at that point.

This revision is now accepted and ready to land.Sep 23 2021, 10:40 AM
This revision was landed with ongoing or failed builds.Sep 27 2021, 12:11 PM
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Fri, Nov 5, 2:08 AM

Final macros will always warn on redefinition, including situations with identical bodies and in system headers.

But

#define NULL syntax error
#pragma clang final(NULL)
#include <stddef.h>
void * p = NULL;

does not produce any warnings/errors for me and indicates that NULL silently gets redefined as a null pointer constant in stddef.h. Is that intended?

does not produce any warnings/errors for me and indicates that NULL silently gets redefined as a null pointer constant in stddef.h. Is that intended?

Nope, that's a bug. Fixed in 3e7ad1f2b2c0a753749eaba88d369d6032a50764. Thank you!