philnik added a comment. In D157572#4606513 <https://reviews.llvm.org/D157572#4606513>, @aaron.ballman wrote:
> In D157572#4604595 <https://reviews.llvm.org/D157572#4604595>, @philnik wrote: > >> In D157572#4604482 <https://reviews.llvm.org/D157572#4604482>, >> @aaron.ballman wrote: >> >>>> This allows standard libraries to mark symbols as extensions, so the >>>> compiler can generate extension warnings when they are used. >>> >>> Huh, so this is basically the opposite of the `__extension__` macro (which >>> is used to silence extension warnings)? >> >> I guess, kind-of. I never really understood the semantics of `__extension__` >> though, so I'm not 100% certain. > > It's used in system headers to say "I'm using an extension here, don't warn > about it in -pedantic mode". Hm, OK. I thought I tried to use it that way at some point and it didn't work in the way I expected. >>> I don't think we need to introduce a new attribute to do this, we already >>> have `diagnose_if`. e.g., https://godbolt.org/z/a5ae4T56o would that >>> suffice? >> >> Part of the idea here is that the diagnostics should be part of >> `-Wc++ab-extension`. > > Hmmm, okay. And I'm assuming `-Wsystem-headers -pedantic` is too chatty > because it's telling the user about all use of extensions, not extensions > being introduced by the library itself? (e.g., > https://godbolt.org/z/Gs3YGheMM is also not what you're after) Yeah, for libc++ we don't support `-Wsystem-headers` in any meaningful way and this doesn't achieve the effect I want. >> I guess we could allow warning flags instead of just `"warning"` and >> `"error"` in `diagnose_if` that specifies which warning group the diagnostic >> should be part of. Something like >> `__attribute__((__diagnose_if__(__cplusplus >= 201703L, "basic_string_view >> is a C++17 extension", "-Wc++17-extensions")))`. I'm not sure how one could >> implement that, but I guess there is some mechanism to translate >> "-Wwhatever" to a warning group, since you can push and pop warnings. That >> would open people up to add a diagnostic to pretty much any warning group. I >> don't know if that's a good idea. I don't really see a problem with that >> other than people writing weird code, but people do that all the time >> anyways. Maybe I'm missing something really problematic though. > > That's actually a pretty interesting idea; `diagnose_if` could be given > another parameter to specify a diagnostic group to associate the diagnostic > with. This would let you do some really handy things like: > > void func(int i) __attribute__((diagnose_if(i < 0, "passing a negative > value to 'func' is deprecated", "warning", "-Wdeprecated"))); > > But if we went this route, would we want to expose other diagnostic-related > knobs like "show in system header" and "default to an error"? Also, the > attribute currently can only be associated with a function; we can use this > for classes by sticking it on a constructor but there's not much help for > putting it on say a namespace or an enumeration. So we may need to extend the > attribute in other ways. CC @cjdb as this seems of interest to you as well. I looked a bit around the code yesterday and it looks like it should be relatively easy to implement. I think we would just have to extend `CustomDiagInfo` to save some more information and pass the warning group in there from the `diagnose_if` sema handler. There are already utilities to translate `-Wwhatever` into the corresponding warning group IDs, so that part is trivial. Given that, I think this sounds like a very interesting way to go. This would also allow us to move the libc++ `atomic` checks to `-Watomic-memory-ordering` and I'm sure there will be more use-cases. Regarding the knobs, I think that would be interesting for some things too. We could add optional string arguments at the end like `"show_in_system_header={false,true}"`, `"default_to_error={false,true}"`, `"enable_by_default={false,true}"`, etc. If they aren't set we probably want to default to `show_in_system_header=false`, `default_to_error=false` and `enable_by_default=true`. Are there any other interesting knobs? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157572/new/ https://reviews.llvm.org/D157572 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
