[PATCH] D137698: [include-cleaner] Add self-contained file support for PragmaIncludes.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked 2 inline comments as done.
hokein added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:39
     FileID FID = SM.getFileID(Loc.physical());
-    const auto *FE = SM.getFileEntryForID(FID);
+    const auto *FE = getResponsibleHeader(FID, SM, PI);
     if (!FE)
----------------
hokein wrote:
> kadircet wrote:
> > i am not sure if sequencing of this and rest of the IWYU pragma handling is the right actually.
> > 
> > e.g. consider a scenario in which:
> > private.h:
> > ```
> > // IWYU private, include "public.h"
> > void foo();
> > ```
> > private-impl.h:
> > ```
> > #pragma once
> > #include "private.h"
> > ```
> > 
> > we'll actually now use `private-impl.h` as provider for `foo` rather than `public.h`, a similar argument goes for export pragma. i think intent of the developer is a lot more explicit when we have these pragmas around, so we should prioritize them and then pick the self contained one, if it's still needed. WDYT?
> It looks reasonable to me. Thanks for raising this case, this is a good point. I mostly considered the case where the self-contained file has the IWYU private mapping.
> 
> My current thought is:
> 
> Overall algorithm: we check the original `FE` to see whether we can get any IWYU mapping (private, export) headers, if there is any, we use these mapping headers as final results; if not, we use the `selfContainedHeader(FE)` and its IWYU mapping headers;
> 
> Below are the cases I considered -- 1), 2), 4) works fine, but the 3) is not perfect (as we prioritize the IWYU mapping headers), but probably ok.
> 
> ```
>   // Case1: Private
>   // findHeaders("private1.inc") => "path/public1.h"
>   Inputs.ExtraFiles["private1.h"] = R"cpp(
>     #pragma once
>     #include "private1.inc"
>   )cpp");
>   Inputs.ExtraFiles["private1.inc"] = R"cpp(
>     // IWYU pragma: private, include "path/public1.h"
>   )cpp";
> 
>   // Case2: Private
>   // findHeaders("private2.inc") => "path/public2.h"
>   Inputs.ExtraFiles["private2.h"] = R"cpp(
>     #pragma once
>     // IWYU pragma: private, include "path/public2.h"
>     #include "private2.inc"
>   )cpp";
>   Inputs.ExtraFiles["private2.inc"] = "";
> 
>   // Case3: Export
>   // findHeaders("details1.inc") => "details1.inc", "export1.h"
>   Inputs.ExtraFiles["export1.h"] = R"cpp(
>     #include "details1.inc" // IWYU pragma: export
>   )cpp";
>   Inputs.ExtraFiles["details1.inc"] = "";
> 
>   // Case4: Export
>   // findHeaders("details2.inc") => "export2.h", "export2_internal.h"
>   Inputs.ExtraFiles["export2.h"] =R"cpp(
>     #pragma once
>     #include "export2_internal.h" // IWYU pragma: export
>   )cpp";
>   Inputs.ExtraFiles["export2_internal.h"] = R"cpp(
>     #pragma once
>     #include "details2.inc"
>   )cpp";
>   Inputs.ExtraFiles["details2.inc"] = "";
> ```
> 
> Let me know what you think about it.
> 
based on our offline discussion, we're going to preserve all header candidates here (and with proper private/export signals attached, which will be in in a followup patch), and let the later step to select proper headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137698