aaron.ballman added inline comments.
================
Comment at: clang/docs/LanguageExtensions.rst:3979
+ #undef FINAL_MACRO // warning: FINAL_MACRO is marked final and should not
be undefined
+ #define FINAL_MACRO // warning: FINAL_MACRO is marked final and should not
be redefined
+
----------------
beanz wrote:
> aaron.ballman wrote:
> > 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!
> > ```
> `-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.
> -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
================
Comment at: clang/test/Lexer/final-macro.c:5-8
+// expected-note@+4{{macro marked 'final' here}}
+// expected-note@+3{{macro marked 'final' here}}
+// expected-note@+2{{macro marked 'final' here}}
+// expected-note@+1{{macro marked 'final' here}}
----------------
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.
================
Comment at: clang/test/Lexer/final-macro.c:10-11
+#pragma clang final(Foo)
+#pragma clang deprecated(Foo)
+#pragma clang header_unsafe(Foo)
+
----------------
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.
================
Comment at: clang/test/Lexer/final-macro.c:14
+// expected-warning@+2{{macro 'Foo' has been marked as final and should not be
redefined}}
+// expected-note@+1{{previous definition is here}}
+#define Foo 1
----------------
This previous definition marker looks wrong to me -- it should be pointing to
line 4, right?
================
Comment at: clang/test/Lexer/final-macro.c:18
+// expected-warning@+2{{macro 'Foo' has been marked as final and should not be
redefined}}
+// expected-warning@+1{{'Foo' macro redefined}}
+#define Foo 2
----------------
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.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108567/new/
https://reviews.llvm.org/D108567
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
