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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:100
 
+  /// Contains all non self-contained files during the parsing.
+  llvm::DenseSet<llvm::sys::fs::UniqueID> NonSelfContainedFiles;
----------------
s/during/detected during/


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:18
+// up the include stack.
+static const FileEntry *getResponsibleHeader(FileID FID,
+                                             const SourceManager &SM,
----------------
what about `selfContainedHeader` instead of `getResponsibleHeader`, responsibility is a concept that we rather define between which header should include what headers (e.g. a header providing return types of its APIs)


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:24
+  while (FE && FID != SM.getMainFileID() && !PI.isSelfContained(FE)) {
+    FID = SM.getFileID(SM.getIncludeLoc(FID));
+    FE = SM.getFileEntryForID(FID);
----------------
`FID =  SM.getDecomposedIncludedLoc(FID).first;`

which does a cached lookup, so might be faster than `getFileID`, especially when performing repeated lookups (e.g. a single file importing multiple tablegen'd headers).


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:37
     // FIXME: Handle macro locations.
     // FIXME: Handle non self-contained files.
     FileID FID = SM.getFileID(Loc.physical());
----------------
drop fixme


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:38
     // FIXME: Handle non self-contained files.
     FileID FID = SM.getFileID(Loc.physical());
+    const auto *FE = getResponsibleHeader(FID, SM, PI);
----------------
can you drop `FID` (as it isn't used elsewhere) and just pass `Loc.physical()` into helper?


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


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:306
 
+bool PragmaIncludes::isSelfContained(const FileEntry *ID) const {
+  return !NonSelfContainedFiles.contains(ID->getUniqueID());
----------------
s/ID/File


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:31
 
+std::string guard(const llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
----------------
drop the `const`


================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:20
 
+std::string guard(const llvm::StringRef Code) {
+  return "#pragma once\n" + Code.str();
----------------
again `const`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137698