[PATCH] D146406: [IncludeCleaner][clangd] Mark umbrella headers as users of private

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146406