sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:210
+ if (!ArgList.hasArgNoClaim(driver::options::OPT__DASH_DASH)) {
+ // In theory there might be more than one input, but clangd can't deal with
+ // them anyway.
----------------
More than one input is a really annoying error to diagnose, would we want to
detect and log it here? Downside is this code runs *really* often.
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:221
- // Check whether the flag exists, either as -flag or -flag=*
- auto Has = [&](llvm::StringRef Flag) {
- for (llvm::StringRef Arg : Cmd) {
- if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
- return true;
- }
- return false;
- };
-
// clangd should not write files to disk, including dependency files
// requested on the command line.
----------------
These are copies of the rendered arg list, i think many of them we could
replace with calls to ArgList.erase before rendering the arg list.
(e.g. syntax-only... if we erased -save-temps and turned off color diagnostics
in Compiler.cpp, I think that covers everything)
... but wait, is "rendering the arglist" even possible?
(I want to expand the scope of this patch unneccesarily, but it might be a way
to offset the extra costs...)
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:232
+ if (ResourceDir &&
+ !ArgList.hasArgNoClaim(driver::options::OPT_resource_dir,
+ driver::options::OPT_resource_dir_EQ))
----------------
Technically the hasArgNoClaim is stale, we've applied edits to the command
since then. Probably fine though, not much use for setting resource-dir. (maybe
we should even drop this check)
hasArgNoClaim is probably cheaper than the old scan we were doing, but its impl
is crazy: not a simple scan, but also not a simple lookup.
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:238
// `--sysroot` is a superset of the `-isysroot` argument.
- if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
+ if (Sysroot && !ArgList.hasArgNoClaim(driver::options::OPT__sysroot,
+ driver::options::OPT__sysroot_EQ,
----------------
OTOH the edits may well set sysroot or isysroot, i'm not sure this is valid :-(
================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:246
if (!ToAppend.empty()) {
Cmd = tooling::getInsertArgumentAdjuster(
std::move(ToAppend), tooling::ArgumentInsertPosition::END)(Cmd, "");
----------------
while here and worrying about efficiency - should we just search for '--'
ourselves?
This is an extra copy (due to the ArgsAdjuster interface)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106527/new/
https://reviews.llvm.org/D106527
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
