[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

This revision was automatically updated to reflect the committed changes. Closed by commit rC361226: [Preamble] Reuse preamble even if an unsaved file does not exist (authored by nik, committed by ). Changed prior to commit: https://reviews.llvm.org/D41005?vs=198809=200415#toc Repository:

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks! LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 198809. nik added a comment. Addressed inline comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 Files: include/clang/Frontend/ASTUnit.h lib/Frontend/ASTUnit.cpp

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added a comment. Again, sorry for the delay. This looks good, just a few NITs from me before I stamp it Comment at: lib/Frontend/PrecompiledPreamble.cpp:457 + llvm::StringMap OverridenFileBuffers; for (const auto : PreprocessorOpts.RemappedFileBuffers) {

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 198656. nik added a comment. Herald added a subscriber: dexonsmith. Minor diff update fixing indentation and removing not needed include. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 186436. nik added a comment. > Meh, something changed in the meanwhile. > ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking > into it. No, it's just me ;) I've referenced the header file wrong. Repository: rC Clang CHANGES SINCE

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Meh, something changed in the meanwhile. ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk fails now. Looking into it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 186429. nik marked an inline comment as done. nik added a comment. Herald added a subscriber: jdoerfert. Herald added a project: clang. Addressed comment. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:459 +if (moveOnNoError(VFS->status(RB.first), Status)) { + OverriddenFiles[Status.getUniqueID()] = PreambleHash; +} else { NIT: remove braces around

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik marked 4 inline comments as done. nik added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:581 + unsigned getPreambleCounter() const { return PreambleCounter; } + ilya-biryukov wrote: > NIT: `getPreambleCounterForTests()`? This is clearly

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 176962. nik marked an inline comment as done. nik added a comment. Addressed comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 Files: include/clang/Frontend/ASTUnit.h

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:581 + unsigned getPreambleCounter() const { return PreambleCounter; } + NIT: `getPreambleCounterForTests()`? This is clearly an internal detail, would try giving it a name that

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41005/new/ https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 174022. nik added a comment. Addressed comments. Repository: rC Clang https://reviews.llvm.org/D41005 Files: include/clang/Frontend/ASTUnit.h lib/Frontend/ASTUnit.cpp lib/Frontend/PrecompiledPreamble.cpp unittests/Frontend/PCHPreambleTest.cpp

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. In https://reviews.llvm.org/D41005#1295632, @ilya-biryukov wrote: > > Before this change, this was not a problem because OverriddenFiles were > > keyed on Status.getUniqueID(). Starting with this change, the key is the > > file path. > > I suggest keeping two maps for

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added a comment. > Before this change, this was not a problem because OverriddenFiles were keyed > on Status.getUniqueID(). Starting with this change, the key is the file path. I suggest keeping two maps for overridden files: one for existing files (key is UniqueID), another one

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Ping. 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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Coming back to this one, I see a failing test: PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble Note that PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble references the header paths in different ways ("//./header1.h" vs

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added a comment. Sorry for the delay, I think I'll come back to this one soon. Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

yvvan added a comment. In https://reviews.llvm.org/D41005#1001854, @cameron314 wrote: > @yvvan: The clang frontend tests (`PCHPreambleTest` and friends) are disabled > on Windows in the makefile (I think because the VFS tests depend on > linux-like paths). So running the tests on Windows

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

cameron314 added a comment. @yvvan: The clang frontend tests (`PCHPreambleTest` and friends) are disabled on Windows in the makefile (I think because the VFS tests depend on linux-like paths). So running the tests on Windows without failures is encouraging but not the whole story.

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:193 /// some number of calls. - unsigned PreambleRebuildCounter; + unsigned PreambleRebuildCountdownCounter; + NIT: Maybe shorten to `PreambleRebuildCountdown`? It's not a

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

yvvan added a comment. No regression in tests on Windows with and without extra patch ([PATCH] Use file path instead of uniqueID) Repository: rC Clang https://reviews.llvm.org/D41005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 128416. nik added a comment. Rebased and renamed the counter variable only. I do not feel comfortable changing the "std::map OverriddenFiles". I can do this in a follow-up change if you want. @Ivan: Coul you please run

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:461 std::map::iterator Overridden = OverriddenFiles.find(Status.getUniqueID()); nik wrote: > ilya-biryukov wrote: > > Will

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + ilya-biryukov wrote: > nik wrote: > > Any better name for this one? Otherwise I

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + nik wrote: > Any better name for this one? Otherwise I would suggest

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + Any better name for this one? Otherwise I would suggest renaming

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik updated this revision to Diff 126353. nik marked 2 inline comments as done. nik added a comment. Addressed Ilya's comments. Repository: rC Clang https://reviews.llvm.org/D41005 Files: include/clang/Frontend/ASTUnit.h lib/Frontend/ASTUnit.cpp lib/Frontend/PrecompiledPreamble.cpp

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added a comment. In https://reviews.llvm.org/D41005#949550, @cameron314 wrote: > It's been a while since I was in this code, but I seem to recall the file > needed to exist on disk in order to uniquely identify it (via inode). Does > this break the up-to-date check? When the

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

cameron314 added a comment. It's been a while since I was in this code, but I seem to recall the file needed to exist on disk in order to uniquely identify it (via inode). Does this break the up-to-date check? Repository: rC Clang https://reviews.llvm.org/D41005

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. This looks useful. Could we avoid creating the `OverlayFileSystem`, though? Comment at: include/clang/Frontend/PrecompiledPreamble.h:109 +

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

nik created this revision. When a preamble is created an unsaved file not existing on disk is already part of PrecompiledPreamble::FilesInPreamble. However, when checking whether the preamble can be re-used, a failed stat of such an unsaved file invalidated the preamble, which led to pointless