adamcz added inline comments.
================
Comment at: clang-tools-extra/clangd/FS.h:91
+IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+getTimeTrackingFS(std::shared_ptr<WallTimer> Timer,
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
----------------
kadircet wrote:
> adamcz wrote:
> > kadircet wrote:
> > > kadircet wrote:
> > > > i feel like this deserves the `FS` specific comment mentioned above.
> > > > maybe something like:
> > > > ```
> > > > This will record all time spent on IO operations in \p Timer.
> > > > ```
> > > i don't think we ever want concurrent access to Timer, i.e. startTime
> > > should never be called without a matching call to stopTime first. passing
> > > it in as a shared_ptr somehow gives the feeling that it might be shared
> > > across multiple objects, which might do whatever they want with the
> > > object.
> > > maybe just pass in a reference ?
> > It's not about concurrency, it's about live time. This timer needs to have
> > the same lifetime as the entire VFS, which is also ref-counted.
> >
> > Alternative would be for the TimerFS to own this timer and expose it's own
> > getTime() method. That means TimerFS must now be visible outside of FS.cpp,
> > but if we're moving it to Preamble.cpp per your other comment that's fine.
> > It's not about concurrency, it's about live time. This timer needs to have
> > the same lifetime as the entire VFS, which is also ref-counted.
>
> Right, I've noticed that as I was going through the rest of the review, but
> forgot to delete these as it was split into two days. sorry for the churn.
>
> > Alternative would be for the TimerFS to own this timer and expose it's own
> > getTime() method. That means TimerFS must now be visible outside of FS.cpp,
> > but if we're moving it to Preamble.cpp per your other comment that's fine.
>
> yes, i think that would be a nice simplification.
> > It's not about concurrency, it's about live time. This timer needs to have
> > the same lifetime as the entire VFS, which is also ref-counted.
>
> Right, I've noticed that as I was going through the rest of the review, but
> forgot to delete these as it was split into two days. sorry for the churn.
>
> > Alternative would be for the TimerFS to own this timer and expose it's own
> > getTime() method. That means TimerFS must now be visible outside of FS.cpp,
> > but if we're moving it to Preamble.cpp per your other comment that's fine.
>
> yes, i think that would be a nice simplification.
Done.
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:389
+ Stats != nullptr ? getTimeTrackingFS(FSTimer, StatCacheFS) : StatCacheFS;
+ FSTimer->stopTimer();
+
----------------
kadircet wrote:
> is this call intentional ?
Oops, no, it's not.
================
Comment at: clang-tools-extra/clangd/Preamble.h:95
const ParseInputs &Inputs, bool StoreInMemory,
+ PreambleBuildStats *Stats,
PreambleParsedCallback PreambleCallback);
----------------
kadircet wrote:
> adamcz wrote:
> > kadircet wrote:
> > > nit: maybe make this the last parameter and default to `nullptr` to get
> > > rid of changes in tests.
> > I'd rather not, unless you insist. Besides not having to modify tests
> > (which I already did anyway), what's the benefit of having it be default?
> > Do you think it's more readable?
> right, i think it's more readable, and moreover it will reduce the need for
> typing that parameter more times in the future (mostly in tests again).
> at the very least, what's the reason for not inserting it at the last
> position but rather before PreambleCallback?
OK, I made it default to nullptr.
The logic behind it not being last was that it's usual (though not a hard rule)
for callbacks to be the last argument, probably to make passing lambdas look
nicer. Not really important.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1008
+ PreambleBuildStats Stats;
+ const bool IsFirstPreamble = !LatestBuild;
LatestBuild = clang::clangd::buildPreamble(
----------------
kadircet wrote:
> adamcz wrote:
> > kadircet wrote:
> > > nit: drop the const per style.
> > Hmm...is that actually documented somewhere? There's definitely many cases
> > of "const bool" in LLVM codebase. I think the "const" improves readability.
> > is that actually documented somewhere
>
> nothing that I can find either.
>
> > There's definitely many cases of "const bool" in LLVM codebase. I think the
> > "const" improves readability.
>
> yes, but I think the majority is still not having "const" in front of bool.
> it's at least the convention in clangd.
> I also agree that the `const` improves readability, but for a local variable
> it carries much less importance and being consistent with the majority of the
> cases here is a lot more important.
> because seeing occurrences of both locals with const and non-const will
> eventually make the reasoning hard and each case surprising.
>
> if you think there's enough value to having consts for locals for
> readability, i think we should figure out a way to make the codebase (at
> least the clangd part) consistent with consts first.
OK, I'll drop const for now then.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121712/new/
https://reviews.llvm.org/D121712
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
