[PATCH] D54691: [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Overall LG. The only concerning bit is that the initialization of the `UFE` is now scattered across the function body.
Grouping `File` and `DeferredOpen` into a struct or adding a comment that those are initialized separately from the rest of the fields might make it more evident what's going on. WDYT?



================
Comment at: include/clang/Basic/FileManager.h:72
   bool InPCH;
-  bool IsValid;               // Is this \c FileEntry initialized and valid?
 
----------------
NIT: not sure if it's actually that useful, but the fact that the comment mentioned that `FileEntry` could be uninitialized if IsValid is false was a hint that drew my attention to this.



================
Comment at: lib/Basic/FileManager.cpp:261
   FileData Data;
   if (getStatValue(InterndFileName, Data, true, openFile ? &F : nullptr)) {
     // There's no real file at the given path.
----------------
Maybe add a comment that DeferredOpen might do an extra stat here that could technically be avoided?
No need to fix this, though, hopefully that won't matter.


================
Comment at: lib/Basic/FileManager.cpp:304
+    }
+    UFE.File = std::move(F);
+  }
----------------
Maybe add an assertion that DeferredOpen is false at this point?
To make it clear we'll never hit this branch twice.


Repository:
  rC Clang

https://reviews.llvm.org/D54691