[clang-tools-extra] d7bba07 - [include-cleaner] Improve header spelling in the presence of links

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2023-01-11T14:44:22+01:00
New Revision: d7bba07526a7298c9331de031dec15daecff3503

URL: https://github.com/llvm/llvm-project/commit/d7bba07526a7298c9331de031dec15daecff3503
DIFF: https://github.com/llvm/llvm-project/commit/d7bba07526a7298c9331de031dec15daecff3503.diff

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

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.

Differential Revision: https://reviews.llvm.org/D141478

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
index a077c26a04b83..cf01e3416dec2 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -90,12 +90,15 @@ class PragmaIncludes {
       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 
diff erent 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.

diff  --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp
index 4df65959011c5..54350df0b00f9 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -204,7 +204,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
         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 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
                                        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 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
     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;