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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment 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)
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137698