[clangd] Fix off-by-one error in CommandMangler

[HighCommander4]

SawInput() is intended to be called for every argument after a --, but it was mistakenly being called for the -- itself.

Partially fixes clangd/clangd#1850

[llvmbot]

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Nathan Ridge (HighCommander4)

Changes

SawInput() is intended to be called for every argument after a --, but it was mistakenly being called for the -- itself.

Partially fixes clangd/clangd#1850

---

Full diff: https://github.com/llvm/llvm-project/pull/160029.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/CompileCommands.cpp (+2-1)
• (modified) clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp (+10)

diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index 80391fe8cce25..c9da98e96ccfb 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -270,7 +270,8 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
   if (auto *DashDash =
           ArgList.getLastArgNoClaim(driver::options::OPT__DASH_DASH)) {
     auto DashDashIndex = DashDash->getIndex() + 1; // +1 accounts for Cmd[0]
-    for (unsigned I = DashDashIndex; I < Cmd.size(); ++I)
+    // Another +1 so we don't treat the `--` itself as an input.
+    for (unsigned I = DashDashIndex + 1; I < Cmd.size(); ++I)
       SawInput(Cmd[I]);
     Cmd.resize(DashDashIndex);
   }
diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index 2ce2975bd962b..e324404e627c2 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -526,6 +526,16 @@ TEST(CommandMangler, RespectsOriginalSysroot) {
                 Not(HasSubstr(testPath("fake/sysroot"))));
   }
 }
+
+TEST(CommandMangler, StdLatestFlag) {
+  const auto Mangler = CommandMangler::forTests();
+  tooling::CompileCommand Cmd;
+  Cmd.CommandLine = {"clang-cl", "/std:c++latest", "--", "/Users/foo.cc"};
+  Mangler(Cmd, "/Users/foo.cc");
+  // Check that the /std:c++latest flag is not dropped
+  EXPECT_THAT(llvm::join(Cmd.CommandLine, " "), HasSubstr("/std:c++latest"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

[HighCommander4]

(Originally submitted as #156593. I'm resubmitting it now on a user branch so I can stack the other part of the fix for clangd/clangd#1850 on top of it.)

[HighCommander4]

As mentioned in the original PR, I'm not sure if this is the best test case for this change, because the fact that transferCompileCommand() drops /std:c++latest is itself a bug which will be fixed in #160030.

But I'm not sure how to write a more direct test for "transferCompileCommand() was not called", and the scenario being tested here is a valid one from a user expectation point of view.

[HighCommander4]

Review ping

[hokein]

good catch, thank you for the fix.