dblaikie added a comment. > In D133425#3778433 <https://reviews.llvm.org/D133425#3778433>, @dblaikie > wrote: > >> So previously/currently-without-this-patch the diagnostic was suppressed if >> the use of ctad was in a system header (suppression based on the >> generic/builtin diagnostic suppression infrastructure) & now it'll suppress >> if that happens, or if the template is defined in a system header. > > One thing I don't understand in the current state of things is why the > diagnostic fires at all inside system headers. I thought warnings in system > headers were discarded?
It doesn't fire if the location of the diagnostic (the place that's using CTAD) is in a system header. But it does fire if that place is outside a system header, but the template that is being used is in a system header (see the SysHeaderObj example above - maybe the naming is confusing? It's a `SysHeaderClass` but the object (`sho`) is not in a system header). > It this whole issue about system headers being added with `-I` instead of > `-isystem`? Don't think so. > FWIW, my "objection" that we should not silence the warning when users try > using CTAD with arbitrary types in `std::` remains -- I think it would be > making a disservice to users to let them use CTAD with classes that have not > been designed with that in mind. At the end of the day, I think I'm > advocating for individual classes opting-out of the warning, while the patch > as currently formulated forces all classes in system headers to be opted-out > of the warning. I think the problem is that not all system headers match the style required for this constraint - so in the interests of compatibility with the complex world of existing system headers, it's suitable not to enforce this stylistic constraint (requiring explicit deduction guides even when the default is what the template author intended to allow CTAD) on system headers. With the knowledge that this does introduce false negatives, but that we don't have a strong enough signal to avoid them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133425/new/ https://reviews.llvm.org/D133425 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
