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@+2{{macro 'Foo' has been marked as final and should not be
redefined}}
+// expected-note@+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@+2{{macro 'Foo' has been marked as final and should not be
redefined}}
+// expected-warning@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
