[clang] NFCI: Use FileEntryRef in SourceManager::FileInfos

[jansvoboda11]

No description provided.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 3e9c36303ca72929275fb97767af43b3aa04cab1 9e605fbaeb72f79307316f5408993c04d4400ed1 -- clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp clang/include/clang/Basic/FileEntry.h clang/include/clang/Basic/SourceManager.h clang/lib/Basic/SourceManager.cpp clang/lib/CodeGen/CGOpenMPRuntime.cpp clang/unittests/Basic/FileEntryTest.cpp

View the diff from clang-format here.

diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h index d6e65966c0ea..7589818b3546 100644 --- a/clang/include/clang/Basic/SourceManager.h +++ b/clang/include/clang/Basic/SourceManager.h @@ -649,7 +649,7 @@ class SourceManager : public RefCountedBase<SourceManager> { /// This map allows us to merge ContentCache entries based /// on their FileEntry*. All ContentCache objects will thus have unique, /// non-null, FileEntry pointers. - llvm::DenseMap<FileEntryRef, SrcMgr::ContentCache*> FileInfos; + llvm::DenseMap<FileEntryRef, SrcMgr::ContentCache *> FileInfos; /// True if the ContentCache for files that are overridden by other /// files, should report the original file name. Defaults to true. 

[benlangmuir]

I'm skeptical that treating FileEntryRef as FileEntry * for the purposes of equality and hashing is a good idea. To me, the obvious interpretation of FileEntryRef == is that the path must be equal, and having it based on the dev/inode is surprising. I know that's not something you're introducing in this PR, but I would prefer we remove that functionality rather than build more on top of it.

Here are some alternative ways we could handle this that I think are more principled:

• We could do DenseMap<FileEntry *, FileEntryRef> instead of the DenseSet, and similarly embed the FileEntryRef into the RHS type in cases with DenseMap
• Alternatively, we could add a wrapper type for FileEntryRef that continues to have the FileEntry * equality/hashing semantics, but where the name of that wrapper and/or the fact you need to unwrap it explicitly makes it obvious that the semantics are not "just" FileEntryRef. If we can figure out a good name for this I think it would be a good solution.

[jansvoboda11]

I understand your concern, we should definitely look into this. However, I think the inode uniquing functionality is pretty fundamental to FileEntry, so I'm not sure it's reasonable for type named FileEntryRef to do anything else. I guess we could split FileEntryRef into two types that make it super clear whether they have the same uniquing semantics or not.

But, since we're only 5 more commits away from finally finishing the whole FileEntry to FileEntryRef transition, I think it would be more pragmatic to keep the status quo in this patch (it's been in place since 2020), finish the transition, and come up with the next steps after that's done. Note that none of the other 5 commits introduce any more usages of FileEntryRef in associative containers.

What do you think?

[benlangmuir]

For what it's worth, until recently there was almost no use of the implicit unique file semantics for FileEntryRef even though the DenseMapInfo had been available for a long time. But okay, I see there are more uses now so this is not the right place to push back on that.

[benlangmuir]

Comment in the test needs a fix, but otherwise LGTM

[benlangmuir]

This comment looks like it's for the other test case. R1Also is inserted first and it wins here.