sammccall added a comment.
Sorry for lots of comments on a fairly small patch.
---
I want to mention one alternative we haven't considered. Ultimately the problem
here is that we have actions that depend on config but we don't know what file
to pull the config from.
This corresponds to passing Path="" to TUScheduler::run (3 calls) or runQuick
(0 such calls).
Our decision is to effectively use the config from the last accessed file.
Why don't we put this policy in TUScheduler, rather than inferring it at the
edges?
Because TUScheduler isn't threadsafe, I think this is as simple as adding a
`std::string LastFile` member, and reading/writing to it in all the run*
methods.
---
> But I'd rather not only store Path in the Config since we might end up
> needing other environment variables in future.
Right. However this patch goes further, and says we must expose *all* other
parameters, in the same way and with the same semantics (since we only get to
document them once).
I don't find the argument totally convincing, because currently we have 2
params, and:
- fresh time shouldn't be exposed at all, unless i'm missing something
- the path is exposed for fairly subtle reasons, and we want to use it
sparingly and so probably give it quite weak semantics (e.g. wouldn't be
appropriate to grab "the current filename" for logging purposes, I think)
================
Comment at: clang-tools-extra/clangd/Config.h:60
+ /// slashes. Empty if not configuring a particular file.
+ llvm::StringRef Path;
+ /// Hint that stale data is OK to improve performance (e.g. avoid IO).
----------------
who owns the underlying string, with what lifetime?
The "root" that creates the config doesn't currently have any obligation to
keep the params around for as long as the config might be active (which can be
async via context). I think in config this should be a std::string
================
Comment at: clang-tools-extra/clangd/Config.h:60
+ /// slashes. Empty if not configuring a particular file.
+ llvm::StringRef Path;
+ /// Hint that stale data is OK to improve performance (e.g. avoid IO).
----------------
sammccall wrote:
> who owns the underlying string, with what lifetime?
>
> The "root" that creates the config doesn't currently have any obligation to
> keep the params around for as long as the config might be active (which can
> be async via context). I think in config this should be a std::string
How do we expect this to be used?
If it's for determining the active project, then I'd expect us to give some
weaker semantic like "representative file from the active project" so we can
fill in e.g. the main file associated with a TU.
But actually we only use the boolean "is this set". Maybe we should just
provide that?
"bool WasConfiguredForPath" or something? Generalizing past that might be
premature.
================
Comment at: clang-tools-extra/clangd/Config.h:69
+ /// Information about the environment used when generating the config.
+ Params Parm;
+
----------------
try to avoid using multiple abbreviations of the same word :-)
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:152
+ // files open from projects with and without an external index.
+ if (C.Index.External || !C.Parm.Path.empty())
+ LastIndex = GetIndexFromSpec(C.Index.External);
----------------
This logic feels unnecessarily tricky: you check whether the spec is set both
here and above.
Consider something longer but more direct
```
SymbolIndex *Specified = C.Index.External ? GetIndexFromSpec(*C.Index.External)
: nullptr;
if (!Specified && C.Parm.Path.empty()) {
// There was no index specified, but we're also not sure which path we're
using.
// Use the index from the last operation that had a valid path.
return LastIndex.
}
// Cache the last-used index, even if it's null ("no external index" is a valid
config!)
LastIndex = Specified;
return Specified;
```
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:154
+ LastIndex = GetIndexFromSpec(C.Index.External);
+ return LastIndex;
}
----------------
this is racy: if there *is* a well-defined external index for the current
config, then we should return it even if another thread is able to concurrently
write to LastIndex.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103179/new/
https://reviews.llvm.org/D103179
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
