[PATCH] D108567: Implement #pragma clang final extension

[Aaron Ballman via Phabricator via cfe-commits]

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 at +4{{macro marked 'final' here}}
+// expected-note at +3{{macro marked 'final' here}}
+// expected-note at +2{{macro marked 'final' here}}
+// expected-note at +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 at +2{{macro 'Foo' has been marked as final and should not be redefined}}
+// expected-note at +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 at +2{{macro 'Foo' has been marked as final and should not be redefined}}
+// expected-warning at +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