Page MenuHomePhabricator

[clangd] Strip /showIncludes in clangd compile commands
ClosedPublic

Authored by aeubanks on Apr 24 2020, 4:16 PM.

Details

Summary

In command lines with /showIncludes, clangd would output includes to stdout, breaking clients.

Fixes https://github.com/clangd/clangd/issues/322.

Diff Detail

Event Timeline

aeubanks created this revision.Apr 24 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2020, 4:16 PM

I did verify that clangd on vscode now works on Windows on the LLVM codebase.

aeubanks retitled this revision from [clangd] Strip /showIncludes in clangd compile commands In command lines with /showIncludes, clangd would output includes to stdout, breaking clients. to [clangd] Strip /showIncludes in clangd compile commands.Apr 24 2020, 4:33 PM
aeubanks edited the summary of this revision. (Show Details)
sammccall accepted this revision.Apr 24 2020, 4:36 PM

Thanks!

clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
82

It's useful to have this test to verify the behavior at the clangd level, but the arg adjuster is a separate library and also used in other contexts.

The unit-test for this arg-adjuster lives in clang/unittests/Tooling/ToolingTest.cpp around line 500. Could you add a test for the new behavior there, too?

This revision is now accepted and ready to land.Apr 24 2020, 4:36 PM
aeubanks updated this revision to Diff 260040.Apr 24 2020, 5:39 PM

Add test in ToolingTest

aeubanks marked an inline comment as done.Apr 24 2020, 5:40 PM
aeubanks added inline comments.
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
82

Done.

kadircet added inline comments.Apr 24 2020, 8:18 PM
clang/lib/Tooling/ArgumentsAdjusters.cpp
101

this can also be --show-includes

kadircet added inline comments.Apr 25 2020, 7:49 AM
clang/lib/Tooling/ArgumentsAdjusters.cpp
101

and looks like there's also /showIncludes:user :/

aeubanks updated this revision to Diff 260103.Apr 25 2020, 8:28 AM

Handle /showIncludes:user

aeubanks marked an inline comment as done.Apr 25 2020, 8:30 AM
aeubanks added inline comments.
clang/lib/Tooling/ArgumentsAdjusters.cpp
101

Handled /showIncludes:user, thanks for catching that. I don't see --show-includes though, clang-cl and clang both don't like the option?

kadircet accepted this revision.Apr 25 2020, 9:37 AM

thanks, lgtm. do you have commit access?

clang/lib/Tooling/ArgumentsAdjusters.cpp
101

ah sorry, that one is a frontend option rather than a driver one. so nvm that one.

thanks, lgtm. do you have commit access?

Yep :)

Thanks for the reviews!

This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
clang/lib/Tooling/ArgumentsAdjusters.cpp
101

clang-cl suports both /foo and -foo style flags. See the top of clang/include/clang/Driver/CLCompatOptions.td

You probably want to add the -showIncludes prefix here too. Ideally this probably wouldn't do its own commandline parsing here (?)