carlosgalvezp added inline comments.

================
Comment at: clang/lib/Sema/SemaExpr.cpp:7161
+        !DiagnosedNestedDesignator && !DiagnosedMixedDesignator &&
+        !getSourceManager().isInSystemMacro(FirstDesignator)) {
       Diag(FirstDesignator, getLangOpts().CPlusPlus20
----------------
rsmith wrote:
> I think we should be checking `Diags.getSuppressSystemWarnings()` before 
> considering whether we're in a system header. (Eg, if we're building libc++ 
> and it enables warnings even in system headers, we want to warn if it uses 
> non-C++ designator syntax.)
> 
> Given that this is something that we will presumably want to do for a bunch 
> of diagnostics, not just this one, I think we should extend the current 
> `ShowInSystemHeader` / `SuppressInSystemHeader` markings in our diagnostics 
> `.td` files to also have a `SuppressInSystemMacro` level, and handle this 
> centrally with the other "suppress warnings in system headers" logic, rather 
> than special-casing this here.
Thanks for the quick feedback!

Fully agree, thanks for the pointers. I tried to apply a similar fix [[ 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/DiagnosticIDs.cpp#L584
 | here ]] but got quite a few tests failing so I thought maybe the intention 
was to apply this on a case-by-case basis.

I'll see what I can do with the `.td` files, thanks!

What unit test file should I use to verify my changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D116833: ... Carlos Galvez via Phabricator via cfe-commits
    • [PATCH] D116... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D116... Carlos Galvez via Phabricator via cfe-commits

Reply via email to