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:252
+
+ llvm::Expected<std::string> makeAbsolute(std::string Path,
+ llvm::StringRef Base,
----------------
consistent with other helpers, can we avoid passing errors around, and instead
issue the diagnostic here and return optional?
Signature seems overly general: the Path argument is always a Located<string>,
the Base is always the fragment directory. These seem likely to always hold.
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:265
+ llvm::SmallString<256> AbsPath;
+ llvm::sys::path::append(AbsPath, FragmentDirectory, Path);
+ llvm::sys::path::native(AbsPath, Style);
----------------
why not path::make_absolute?
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:292
+ Spec.Kind = Config::ExternalIndexSpec::File;
+ // Make sure File is an absolute native path.
+ auto AbsPath = makeAbsolute(std::move(**External.File),
FragmentDirectory,
----------------
comment echoes the code, which is not hard to read - remove?
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:256
+ diag(Error, "At least one of File or Server must be specified.",
+ llvm::SMRange());
+ return;
----------------
kadircet wrote:
> sammccall wrote:
> > these missing ranges are potentially pretty difficult for users - they
> > won't know e.g. which file the error occurred in.
> >
> > Do you think we can `Located<>` the whole `ExternalBlock`?
> I was doing that initially, but they felt similarly annoying, as they point
> at value corresponding to `External` block rather than the whole {key,value}
> range. In addition to that it only covers the first line :/
>
> Happy to do that if you think that's still better than an empty/invalid range.
I think it's better than the empty range, even if it just gives us a location
for the block, that tells us which file and which fragment.
(sorry, not sure what "`,value` range" is...)
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:305
+
+ // Disable background indexing for the files under the mountpoint.
+ // Note that this will overwrite statements in any previous fragments
----------------
kadircet wrote:
> sammccall wrote:
> > hang on, you say "under the mountpoint", but... that's not what's happening.
> >
> > If we want that behavior, we need to use SourceInfo::Directory as the
> > mountpoint (and so probably give users a way to set that in the global
> > config fragment).
> > hang on, you say "under the mountpoint", but... that's not what's happening.
>
> Umm, why? We bail-out at the beginning of apply if filepath doesn't start
> with mountpoint ?
Sorry, I was confused...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90749/new/
https://reviews.llvm.org/D90749
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
