[PATCH] D108567: Implement #pragma clang final extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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



================
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
----------------
beanz wrote:
> aaron.ballman wrote:
> > This previous definition marker looks wrong to me -- it should be pointing to line 4, right?
> 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...
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!


================
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
----------------
beanz wrote:
> aaron.ballman wrote:
> > 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.)
> 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.
I think the current behavior is defensible; if a user has a real world problem with it, we can address it at that point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108567/new/

https://reviews.llvm.org/D108567