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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
