sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:75
llvm::SourceMgr *SourceMgr;
+ // Directory containing the fragment. Absolute path with forward slashes,
with
+ // a trailing slash or empty for user-config fragments.
----------------
nit: associated with again (or just "normalized
Fragment::SourceInfo::Directory")
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:162
return false;
+ llvm::StringRef Path = P.Path;
+ // Ignore the file if it is not nested under Fragment and strip the
----------------
maybe pull out a method `Optional<StringRef> configRelative(StringRef)`?
This would avoid the duplication
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:168
+ if (!FragmentDir.empty())
+ Path.consume_front("/");
return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
----------------
not needed as you've ensured FragmentDir ends with a slash
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+ // Directory associated with this fragment.
+ std::string Dir;
----------------
nit: Directory matching Fragment.Source.Directory?
(the abbreviation isn't terrible but consistency is nice)
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:124
std::vector<CompiledFragment> Result;
- Cache.read(FS, DC, P.FreshTime, Result);
+ Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result);
return Result;
----------------
kadircet wrote:
> sammccall wrote:
> > this seems like a surprising place to encode this choice (why is
> > fromYAMLFile coupled to the idea that it's a global config file not
> > associated with any dir?)
> It was mostly because the only user of `fromYAMLFile` was user-config
> provider. Would you rather have this as an additional parameter instead?
>
> I can't see any other way of updating that information directly from the
> user, apart from changing the interface of `Provider` to have some sort of
> `setPathSpec` method.
Yes, I'd prefer this to be a parameter (I suppose the dir seems slightly
cleaner than a boolean).
The fact that user-config is the only user seems like a coincidence, I'd rather
express the decision at that level.
(On the other hand, the fact that RelFileProvider *does* set Dir isn't a
coincidence, it seems OK to hardcode that)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90270/new/
https://reviews.llvm.org/D90270
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
