This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f059a258a1d: [clangd] Handle absolute/relative path
specifications in Config (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES
kadircet updated this revision to Diff 301912.
kadircet added a comment.
- Explicitly handle FragmentDir.empty() case by returning Path as-is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90270/new/
https://reviews.llvm.org/D90270
Files:
clang-
kadircet marked 2 inline comments as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:170
+ [PathMatch(std::move(PathMatch)),
+ FragmentDir(FragmentDirectory)](const Params &P) {
if (P.Path.empty())
-
sammccall added a comment.
Still LG but a few more nits
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:46
+// Returns an empty stringref if Path is not under FragmentDir.
+llvm::StringRef configRelative(llvm::StringRef Path,
you should document+verify
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:51
+return llvm::StringRef();
+ return Path;
+}
sammccall wrote:
> not that if Path == FragmentDir you're going to return "" which means "not
> under".
> I don't think th
kadircet updated this revision to Diff 301842.
kadircet marked an inline comment as done.
kadircet added a comment.
- Return "." if Path == FragmentDir in conifgRelative.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90270/new/
https://reviews.llvm
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:51
+return llvm::StringRef();
+ return Path;
+}
not that if Path == FragmentDir you're going to return "" which means "not
under".
I don't think this can happen yet, but
kadircet added inline comments.
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
sammccall wrote:
>
kadircet updated this revision to Diff 301311.
kadircet marked 6 inline comments as done.
kadircet added a comment.
- Make Directory a parameter of fromYAMLFile.
- Factor out configRelative logic to a helper.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm
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
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:124
std::vector Result;
- Cache.read(FS, DC, P.FreshTime, Result);
+ Cache.read(FS, DC, P.FreshTime, Fragment::SourceInfo::Absolute, Result);
return Result;
---
kadircet updated this revision to Diff 301280.
kadircet marked 9 inline comments as done.
kadircet added a comment.
- Drop PathSpec, deduce it from emptyness of FragmentDirectory.
- Make tests more homogenous.
- Add path cannonicalization to compile stage.
Repository:
rG LLVM Github Monorepo
sammccall added a comment.
Thanks for doing this!
Main points:
- I think we should have one high-level concept of "the optional associated
directory" rather than two low-level ones of "the location of the config file"
and "should we use the location to form relative paths"
- We need to be care
kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kadircet requested review of this revision.
Herald added subscribers: ormris, MaskRay, ilya-biryukov.
This introduces a mechanism for pro
14 matches
Mail list logo
