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&id=200415#toc
Repository:
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
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
lib/Frontend
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 &RB : PreprocessorOpts.RemappedFileBuffers)
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/
https://reviews.llvm.org/D4
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
https://lists.llvm.org/cgi-bin/mailman/listi
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
https://lists.llvm.org/cgi-bin/mailman/listi
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 LA
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
___
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/
https://reviews.l
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 single-statemen
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 an
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
lib/Fron
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
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
http://lists.llvm.org/cgi-bin/mailman/listin
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
http://lists.llvm.org/cgi-bin/mailman/listin
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
Index:
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 overr
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 f
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
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
"//./foo/../head
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
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 withou
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.
Repository:
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 grea
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
http:/
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 the tests with this change on Windows?! If
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 anything fail if we remove the map from `UniqueI
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
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 r
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
PreambleRebuil
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
u
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 th
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 fil
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
_
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
+ std::chr
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 and
37 matches
Mail list logo
