sammccall added a comment.
Kadir and I discussed this a bunch today. FWIW I agree with this patch:
sticking the query-driver step in the middle of the pipeline is the right way
to solve these bugs.
Putting it on the end and hoping to get away with it feels risky to me.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:477
std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
- BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
- std::move(BaseCDB));
+ Extractor =
+ getSystemIncludeExtractor(llvm::makeArrayRef(Opts.QueryDriverGlobs));
----------------
this is carefully preserving existing behavior: --query-driver has no effect
when --compile_commands_from=lsp
I don't think that behavior is intended, we can remove it either here or in a
followup patch
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:79
+ /// Methods exposed for testing.
+ llvm::Optional<tooling::CompileCommand> getCompileCommand(PathRef File);
----------------
(if the unit tests gets converted back to a lit test, we should probably remove
this)
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:310
+ // that needs to be invoked may come from the CompileFlags->Compiler key
+ // - BEFORE resolveDriver() because that can mess up the driver path,
+ // e.g. changing gcc to /path/to/clang/bin/gcc
----------------
kadircet wrote:
> why is this necessary? i.e. are there cases where heuristics in clang-driver
> today cannot identify the same set of includes we would extract from a system
> installed `gcc` ?
>
> because if we were to get rid of this requirement, we can just run this as an
> extra arg adjuster after running command mangler, without changing the logic
> in CommandMangler at all (and also rendering the underlying patch unnecessary
> (i am still not sure if it's needed, but need to take a closer look there)).
I think Kadir is suggesting allowing a bug (handling of `gcc` +
`--query-driver`) that we think is unimportant, in order to simplify the
implementation.
After staring at this for a while, I think having the CommandMangler
encapsulate all the ugly stuff we do here is the right *interface*, and that
justifies the implementation being a bit ugly.
================
Comment at: clang-tools-extra/clangd/CompileCommands.h:56
private:
+ std::unique_ptr<CompileCommandsAdjuster> SystemIncludeExtractor;
Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
----------------
caches notwithstanding, CommandMangler is a "simple struct" - I think this
field should be public for consistency.
(And set manually rather than passed to detect() - there's no detection
involved)
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:318
/// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor : public CompileCommandsAdjuster {
public:
----------------
this is renaming the class, but not the file, the flag, etc.
The actual name aside, such a rename should probably be more complete and a
separate commit. If we keep the `--query-driver` flag we'll have at least two
names, so we have to weigh that against the betterness of the name too.
(I agree that the current name is somewhat confusing. The new name is somewhat
inaccurate: we're extracting from a compiler rather than the (operating?)
system, and we extract more than includes. I prefer the new one on balance but
maybe `ToolchainFlagsExtractor` or something would be even better...)
================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:74
FeatureModuleSet FeatureModules;
+ llvm::Optional<ClangdLSPServer> Server;
----------------
The new destruction order looks much less obvious, even if we can get away with
it at the moment.
Maybe add an accessor instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133757/new/
https://reviews.llvm.org/D133757
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
