[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Basic/FileManager.h:316
+  void getSeenFileEntries(
+      SmallVectorImpl<OptionalFileEntryRefDegradesToFileEntryPtr> &Entries)
+      const;
----------------
rmaz wrote:
> jansvoboda11 wrote:
> > Since we're already modifying the two only users of this function, maybe we could use `Optional<FileEntryRef>` instead of `OptionalFileEntryRefDegradesToFileEntryPtr` (which we want to eventually remove)?
> We could do that with less churn once more or the called methods in the loop over the files are refactored in D142724, but I'm fine with either approach.
Hmm, thinking about this more, it doesn't make sense for this function to return empty `Optional<FileEntryRef>`. Changing the parameter type to `SmallVectorImpl<FileEntryRef> &` and updating the clients to use `.` instead of `->` is the most clear/correct option IMO.

I think the function name & documentation should point out that we're only returning files non-vfs-redirected files (and vfs-redirected files that don't use the external name).


================
Comment at: clang/lib/Basic/FileManager.cpp:20
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileSystemStatCache.h"
----------------
Already included in `FileManager.h`.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:39
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
----------------
Already provided by `FileManager.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143414