nik marked 2 inline comments as done. nik added inline comments.
================ Comment at: include/clang/Frontend/PrecompiledPreamble.h:109 + std::chrono::steady_clock::time_point getCreationTimePoint() const { + return CreationTimePoint; ---------------- ilya-biryukov wrote: > Having this time stamp in the interface of `PrecompiledPreamble` doesn't > really makes sense. > > I propose we remove this method and test in a different way instead. > > For example, we could add a counter to `ASTUnit` that increases when preamble > is built and check this counter. > Or we could add a unit-test that uses `PrecompiledPreamble` directly. > For example, we could add a counter to ASTUnit that increases when preamble > is built and check this counter. OK, I've followed this proposal because there is already test infrastructure in PCHPreambleTest that I can easily reuse. ================ Comment at: lib/Frontend/PrecompiledPreamble.cpp:437 + vfs::OverlayFileSystem OFS(VFS); + IntrusiveRefCntPtr<vfs::InMemoryFileSystem> IMFS( ---------------- ilya-biryukov wrote: > Can we do the same thing without creating an additional `OverlayFileSystem`? > > If we add a separate map to check for those: > ``` > llvm::StringMap<PreambleFileHash> OverriddenFilesWithoutFS. > // Populate the map with paths not existing in VFS. > > for (const auto &F : FilesInPreamble) { > vfs::Status Status; > if (!moveOnNoError(OFS.status(F.first()), Status)) { > // If we can't stat the file, check whether it was overriden > auto It = OverriddenFilesWithoutFS[F.first()]; > if (It == OverriddenFilesWithoutFS.end()) > return false; > //..... > } > //...... > > } > ``` > Can we do the same thing without creating an additional OverlayFileSystem? Hmm, I thought I had a good reason for this one. I don't remember it and the test still passes, so I did it without it now. Is there any real advantage in first trying the stat, then checking OverriddenFilesWithoutFS? Since I don't see any, I've changed order because the stat can then be avoided; also, it removes some nesting. ================ Comment at: lib/Frontend/PrecompiledPreamble.cpp:444 vfs::Status Status; - if (!moveOnNoError(VFS->status(RB.first), Status)) - return false; - + assert(moveOnNoError(IMFS->status(RB.first), Status)); OverriddenFiles[Status.getUniqueID()] = ---------------- ilya-biryukov wrote: > `assert` macro is a no-op when `NDEBUG` is defined. > One must never put an operation with side-effects inside `assert`. Huch, forgot to remove it on cleanup. Anyway, it's obsolete now. Repository: rC Clang https://reviews.llvm.org/D41005 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits