kadircet marked 2 inline comments as done.
kadircet added a comment.
thanks a lot for the review!
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:100
+ // Check if main file is the public interface for a private header. If so
+ // we shouldn't diagnose it as unused.
+ if (auto PHeader = PI->getPublic(I.Resolved); !PHeader.empty()) {
----------------
hokein wrote:
> kadircet wrote:
> > hokein wrote:
> > > The beahvior (no showing the diagnostic) seems reasonable to me as we can
> > > infer that the included header is supposed to be exported by the main
> > > file. Just explore this a bit more.
> > >
> > > The sad bit is that we start diverging from the classical IWYU tool (I
> > > have check it, for this case, it gives an unused-include unless you add
> > > an export pragma).
> > >
> > > I think the main cause is that we're missing the `// IWYU pragma:
> > > export`, we should still want to recommend users to add the pragma
> > > export, right?
> > >
> > > My only concern is that without the `export` pragma, whether the include
> > > is used can't be reason about by just "reading" the main-file content by
> > > human, e.g. for the case below, there is no usage of the `private.h`
> > > (previously the trailing `// IWYU pragma: export` comment is considered a
> > > usage), we have to open the private.h and find the private mapping to
> > > verify the usage.
> > >
> > > ```
> > > // public.h
> > >
> > > #include "private.h"
> > > ```
> > >
> > > It seems better to provide an `adding //IWYU pragma: export` FIXIT.
> > >
> > > The sad bit is that we start diverging from the classical IWYU tool (I
> > > have check it, for this case, it gives an unused-include unless you add
> > > an export pragma).
> >
> > we're not providing the same behavior with IWYU tool, it's nice to be
> > compatible with it and this change won't break that compatibility (i.e. if
> > IWYU tool drops this include, we're not suggesting to insert or while IWYU
> > tool is suggesting to insert we're not suggesting to remove). so i don't
> > think this argument applies here.
> >
> > > I think the main cause is that we're missing the // IWYU pragma: export,
> > > we should still want to recommend users to add the pragma export, right?
> >
> > i don't see the reason why. i think we're actually losing value by
> > enforcing people to annotate headers in both places. if the library
> > provider already put a pragma in the included header, marking the umbrella
> > header as the private header, what's the extra value we're getting by
> > making them also annotate the public header with a bunch of export pragmas?
> > i feel like this goes against the nature of letting tooling automate things
> > as much as possible.
> >
> > > My only concern is that without the export pragma, whether the include is
> > > used can't be reason about by just "reading" the main-file content by
> > > human, e.g. for the case below, there is no usage of the private.h
> > > (previously the trailing // IWYU pragma: export comment is considered a
> > > usage), we have to open the private.h and find the private mapping to
> > > verify the usage.
> >
> > this definitely makes sense, and having an explicit IWYU export/keep pragma
> > on these includes are definitely nice but I believe in the common case,
> > because private-public matching is 1:1, the reason why a private header is
> > included inside a public header is quite obvious hence absence of these
> > pragmas won't really cause confusion, to the contrary forcing people to
> > introduce them will actually create frustration.
> >
> > WDYT?
> > this change won't break that compatibility (i.e. if IWYU tool drops this
> > include, we're not suggesting to insert or while IWYU tool is suggesting to
> > insert we're not suggesting to remove). so i don't think this argument
> > applies here.
>
> Agree on this point. This is a safe change. But now clangd is diverging from
> the clang-include-cleaner tool.
>
> > i feel like this goes against the nature of letting tooling automate things
> > as much as possible
>
> I think the proposed change is good.
>
> From the tooling automation, for example, we're doing a large cleanup (remove
> unused includes), we should not remove this include.
>
> For interactive tools (like clangd), I actually think in addition to that,
> clangd should tell the user that they are missing an IWYU export pragma here,
> and suggest a FIXIT (but that's probably another thread).
>
> > but I believe in the common case, because private-public matching is 1:1,
> > the reason why a private header is included inside a public header is quite
> > obvious hence absence of these pragmas won't really cause confusion
>
> I'm not sure this is true, I think private=>public is 1:1, but
> public=>private is not always 1:1 (e.g. public header use
> `begin_export`/`end_export` to export a bunch of headers),
> for these public headers, missing export pragmas is hard to justify the
> purpose.
>
>
>
> But now clangd is diverging from the clang-include-cleaner tool.
And I was trying to make the point that we're already not trying to avoid that,
deliberately. As our use definitions & objectives around automation are
drastically different.
> For interactive tools (like clangd), I actually think in addition to that,
> clangd should tell the user that they are missing an IWYU export pragma here,
> and suggest a FIXIT (but that's probably another thread).
I think there's harm rather than value in enabling people to introduce
annotations, especially when they're not needed. Happy to discuss this more.
> I think private=>public is 1:1, but public=>private is not always 1:1
There's no matching like public "header" => private "header". The mapping goes
from "an include in a header" to "a private header", which is still 1:1 and I
believe even in that case it's obvious (regular case for an umbrella header)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146406/new/
https://reviews.llvm.org/D146406
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
