[PATCH] D141478: [include-cleaner] Improve header spelling in the presence of links

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: kadircet, hokein.
Herald added a project: All.
sammccall requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

HeaderSearch uses FileEntry::getName() to determine the best spelling of a
header. FileEntry::getName() is now the name of the *last* retrieved ref.
This means that when FileManager::getFile() hits an existing inode through a new
path, it changes the spelling of that header.

In the absence of explicit logic to track the preferred name(s) of header files,
we should avoid gratuitously calling getFile() with paths different than how
the header was originally included, such as the result of realpath().

The originally-specified path should be fine here:

- if the same filemanager is being used for record/analysis, we'll hit the filename cache
- if a different filemanager is being used e.g. preamble scenario, we should get the same result unless either the working directory has changed (which it shouldn't, else many other things will fail) or the file has gone/changed inode (in which case the old method doesn't work either)

Needless to say this is fragile, but talking to @kadircet offline, it's good
enough for our purposes for now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141478

Files:
  clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
  clang-tools-extra/include-cleaner/lib/Record.cpp


Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -204,7 +204,7 @@
         Top.SeenAtLine == HashLine) {
       if (IncludedHeader)
         Out->IWYUExportBy[IncludedHeader->getUniqueID()].push_back(
-            Top.FullPath);
+            Top.Path);
       // main-file #include with export pragma should never be removed.
       if (Top.SeenAtFile == SM.getMainFileID())
         Out->ShouldKeep.insert(HashLine);
@@ -251,14 +251,13 @@
                                        SM.getFileOffset(Range.getBegin()));
     // Record export pragma.
     if (Pragma->startswith("export")) {
-      ExportStack.push_back(
-          {CommentLine, CommentFID,
-           save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()),
-           false});
+      ExportStack.push_back({CommentLine, CommentFID,
+                             save(SM.getFileEntryForID(CommentFID)->getName()),
+                             false});
     } else if (Pragma->startswith("begin_exports")) {
-      ExportStack.push_back(
-          {CommentLine, CommentFID,
-           save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()), true});
+      ExportStack.push_back({CommentLine, CommentFID,
+                             save(SM.getFileEntryForID(CommentFID)->getName()),
+                             true});
     } else if (Pragma->startswith("end_exports")) {
       // FIXME: be robust on unmatching cases. We should only pop the stack if
       // the begin_exports and end_exports is in the same file.
@@ -297,8 +296,8 @@
     int SeenAtLine = 0; // 1-based line number.
     // The file where we saw the pragma.
     FileID SeenAtFile;
-    // FullPath of the file SeenAtFile.
-    StringRef FullPath;
+    // Name (per FileEntry::getName()) of the file SeenAtFile.
+    StringRef Path;
     // true if it is a block begin/end_exports pragma; false if it is a
     // single-line export pragma.
     bool Block = false;
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===================================================================
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -90,12 +90,15 @@
       IWYUPublic;
 
   /// A reverse map from the underlying header to its exporter headers.
-  //
-  //  There's no way to get a FileEntry from a UniqueID, especially when it
-  //  hasn't been opened before. So store the full path and convert it to a
-  //  FileEntry by opening the file again through a FileManager.
+  ///
+  /// There's no way to get a FileEntry from a UniqueID, especially when it
+  /// hasn't been opened before. So store the path and convert it to a
+  /// FileEntry by opening the file again through a FileManager.
+  ///
+  /// We don't use RealPathName, as opening the file through a different name
+  /// changes its preferred name. Clearly this is fragile!
   llvm::DenseMap<llvm::sys::fs::UniqueID,
-                 llvm::SmallVector</*RealPathNames*/ llvm::StringRef>>
+                 llvm::SmallVector</*FileEntry::getName()*/ llvm::StringRef>>
       IWYUExportBy;
 
   /// Contains all non self-contained files detected during the parsing.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141478.488134.patch
Type: text/x-patch
Size: 3405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230111/150a7514/attachment-0001.bin>