Skip to content

Commit d7bba07

Browse files
committed
[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
1 parent 60cdd12 commit d7bba07

File tree

2 files changed

+17
-15
lines changed

2 files changed

+17
-15
lines changed

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,12 +90,15 @@ class PragmaIncludes {
9090
IWYUPublic;
9191

9292
/// A reverse map from the underlying header to its exporter headers.
93-
//
94-
// There's no way to get a FileEntry from a UniqueID, especially when it
95-
// hasn't been opened before. So store the full path and convert it to a
96-
// FileEntry by opening the file again through a FileManager.
93+
///
94+
/// There's no way to get a FileEntry from a UniqueID, especially when it
95+
/// hasn't been opened before. So store the path and convert it to a
96+
/// FileEntry by opening the file again through a FileManager.
97+
///
98+
/// We don't use RealPathName, as opening the file through a different name
99+
/// changes its preferred name. Clearly this is fragile!
97100
llvm::DenseMap<llvm::sys::fs::UniqueID,
98-
llvm::SmallVector</*RealPathNames*/ llvm::StringRef>>
101+
llvm::SmallVector</*FileEntry::getName()*/ llvm::StringRef>>
99102
IWYUExportBy;
100103

101104
/// Contains all non self-contained files detected during the parsing.

clang-tools-extra/include-cleaner/lib/Record.cpp

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
204204
Top.SeenAtLine == HashLine) {
205205
if (IncludedHeader)
206206
Out->IWYUExportBy[IncludedHeader->getUniqueID()].push_back(
207-
Top.FullPath);
207+
Top.Path);
208208
// main-file #include with export pragma should never be removed.
209209
if (Top.SeenAtFile == SM.getMainFileID())
210210
Out->ShouldKeep.insert(HashLine);
@@ -251,14 +251,13 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
251251
SM.getFileOffset(Range.getBegin()));
252252
// Record export pragma.
253253
if (Pragma->startswith("export")) {
254-
ExportStack.push_back(
255-
{CommentLine, CommentFID,
256-
save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()),
257-
false});
254+
ExportStack.push_back({CommentLine, CommentFID,
255+
save(SM.getFileEntryForID(CommentFID)->getName()),
256+
false});
258257
} else if (Pragma->startswith("begin_exports")) {
259-
ExportStack.push_back(
260-
{CommentLine, CommentFID,
261-
save(SM.getFileEntryForID(CommentFID)->tryGetRealPathName()), true});
258+
ExportStack.push_back({CommentLine, CommentFID,
259+
save(SM.getFileEntryForID(CommentFID)->getName()),
260+
true});
262261
} else if (Pragma->startswith("end_exports")) {
263262
// FIXME: be robust on unmatching cases. We should only pop the stack if
264263
// the begin_exports and end_exports is in the same file.
@@ -297,8 +296,8 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
297296
int SeenAtLine = 0; // 1-based line number.
298297
// The file where we saw the pragma.
299298
FileID SeenAtFile;
300-
// FullPath of the file SeenAtFile.
301-
StringRef FullPath;
299+
// Name (per FileEntry::getName()) of the file SeenAtFile.
300+
StringRef Path;
302301
// true if it is a block begin/end_exports pragma; false if it is a
303302
// single-line export pragma.
304303
bool Block = false;

0 commit comments

Comments
 (0)