hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
================
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()) {
----------------
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.
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:231
+ {
+ Inputs.Code = R"cpp(
+#include "private.h"
----------------
nit: fix the format, or just use a string.
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:232
+ Inputs.Code = R"cpp(
+#include "private.h"
+)cpp";
----------------
nit: add a trailing comment `// line 2`, to make the L243 clearer.
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
