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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
