[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
ychen added inline comments. Comment at: clang-tools-extra/clangd/CompileCommands.cpp:356 + HELP, METAVAR, VALUES) \ + if (DriverID::OPT_##ALIAS != DriverID::OPT_INVALID && ALIASARGS == nullptr) \ +AddAlias(DriverID::OPT_##ID, DriverID::OPT_##ALIAS); \ This emits a huge number of `-Wtautological-constant-compare` warnings due to ALIAS being OPT_INVALID etc. Enclose this with pragma to disable these warnings? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81958/new/ https://reviews.llvm.org/D81958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall marked an inline comment as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:15
#include "clang/Tooling/ArgumentsAdjusters.h"
+#include "llvm/Option/Option.h"
+#include "llvm/Support/Allocator.h"
daltenty wrote:
> This breaks the powerpc64le bots. Looks like we aren't linking against Option:
>
> ```
> CompileCommands.cpp:(.text._ZZN5clang6clangd11ArgStripper8rulesForEN4llvm9StringRefEENK3$_3clEv+0x514c):
> undefined reference to
> `llvm::opt::OptTable::getOption(llvm::opt::OptSpecifier) const'
> ```
>
> http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/5697/
Thanks! 50a5fa8b9ba4b09433bf46f4228d4e4cae9ac486 will hopefully fix.
(Not sure why this didn't show up on x64 but linking is mostly a mystery to
me...)
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > hokein wrote:
> > > > > add tests for stripping the diagnostic flags, `-Wfoo` etc.
> > > > Those aren't actually separate flags: "-W" is a Joined flag and foo is
> > > > an arg.
> > > > Do you want a test specifically for such a string anyway?
> > > > Or do you want special behavior for them? (Like interaction between
> > > > -Wfoo and -Wno-foo)
> > > I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> > > would be nice to have them in tests as I think these are important cases.
> > >
> > > > Those aren't actually separate flags: "-W" is a Joined flag and foo is
> > > > an arg.
> > >
> > > oh, if I understand correctly:
> > >
> > > - `strip("unused", "clang -Wunused foo.cc")` => `clang foo.cc` ?
> > > - `strip("-W", "clang -Wunused -Wextra foo.cc")` => `clang foo.cc` //
> > > remove all -W flags ?
> > > - `strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> > > foo.cc")` => `clang -Wunused -Werror=date-time foo.cc`?
> > >
> > > I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> > > would be nice to have them in tests as I think these are important cases.
> >
> > Added.
> >
> > > strip("unused", "clang -Wunused foo.cc") => clang foo.cc ?
> >
> > No, we only strip clang args or flag names, not flag args (confusing
> > terminology).
> > -W will strip -Wunused (flag name). -Wunused will strip -Wunused (clang
> > arg). -W* will strip -Wunused (clang arg). but "unused" doesn't match -
> > it's not the name of a flag, and it's not the arg either.
> >
> > > strip("-W", "clang -Wunused -Wextra foo.cc") => clang foo.cc // remove
> > > all -W flags ?
> >
> > Yes
> >
> > > strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> > > foo.cc") => clang -Wunused -Werror=date-time foo.cc?
> >
> > No, again we don't strip flag args.
> > (It's not clear how useful/easy to use this would be, maybe we can
> > re-examine later?)
> > No, we only strip clang args or flag names, not flag args (confusing
> > terminology).
> > -W will strip -Wunused (flag name). -Wunused will strip -Wunused (clang
> > arg). -W* will strip -Wunused (clang arg). but "unused" doesn't match -
> > it's not the name of a flag, and it's not the arg either.
>
> oh, I was confused by these terms :(
>
> > No, again we don't strip flag args.
> > (It's not clear how useful/easy to use this would be, maybe we can
> > re-examine later?)
>
> we can still strip it by passing "-Werror-unused" arg, right? if so, I think
> it should be fine.
> oh, I was confused by these terms :(
They're confusing. Tried to clarify in the doc.
> we can still strip it by passing "-Werror-unused" arg, right? if so, I think
> it should be fine.
Yeah. (-Werror=unused I guess, but in any case...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
daltenty added inline comments. Comment at: clang-tools-extra/clangd/CompileCommands.cpp:15 #include "clang/Tooling/ArgumentsAdjusters.h" +#include "llvm/Option/Option.h" +#include "llvm/Support/Allocator.h" This breaks the powerpc64le bots. Looks like we aren't linking against Option: ``` CompileCommands.cpp:(.text._ZZN5clang6clangd11ArgStripper8rulesForEN4llvm9StringRefEENK3$_3clEv+0x514c): undefined reference to `llvm::opt::OptTable::getOption(llvm::opt::OptSpecifier) const' ``` http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/5697/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81958/new/ https://reviews.llvm.org/D81958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG8eb8c92eb469: [clangd] Add library to semantically strip
flags by name. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D81958?vs=277728=277810#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,166 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete args if we've matched the target option.
+ EXPECT_EQ(strip("-I", "clang -Xclang"), "clang -Xclang");
+ EXPECT_EQ(strip("-I", "clang -Xclang -I"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I -Xclang"), "clang");
+
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
hokein accepted this revision.
hokein added a comment.
still lg.
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > add tests for stripping the diagnostic flags, `-Wfoo` etc.
> > > Those aren't actually separate flags: "-W" is a Joined flag and foo is an
> > > arg.
> > > Do you want a test specifically for such a string anyway?
> > > Or do you want special behavior for them? (Like interaction between -Wfoo
> > > and -Wno-foo)
> > I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> > would be nice to have them in tests as I think these are important cases.
> >
> > > Those aren't actually separate flags: "-W" is a Joined flag and foo is an
> > > arg.
> >
> > oh, if I understand correctly:
> >
> > - `strip("unused", "clang -Wunused foo.cc")` => `clang foo.cc` ?
> > - `strip("-W", "clang -Wunused -Wextra foo.cc")` => `clang foo.cc` //
> > remove all -W flags ?
> > - `strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> > foo.cc")` => `clang -Wunused -Werror=date-time foo.cc`?
> >
> > I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> > would be nice to have them in tests as I think these are important cases.
>
> Added.
>
> > strip("unused", "clang -Wunused foo.cc") => clang foo.cc ?
>
> No, we only strip clang args or flag names, not flag args (confusing
> terminology).
> -W will strip -Wunused (flag name). -Wunused will strip -Wunused (clang arg).
> -W* will strip -Wunused (clang arg). but "unused" doesn't match - it's not
> the name of a flag, and it's not the arg either.
>
> > strip("-W", "clang -Wunused -Wextra foo.cc") => clang foo.cc // remove all
> > -W flags ?
>
> Yes
>
> > strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> > foo.cc") => clang -Wunused -Werror=date-time foo.cc?
>
> No, again we don't strip flag args.
> (It's not clear how useful/easy to use this would be, maybe we can re-examine
> later?)
> No, we only strip clang args or flag names, not flag args (confusing
> terminology).
> -W will strip -Wunused (flag name). -Wunused will strip -Wunused (clang arg).
> -W* will strip -Wunused (clang arg). but "unused" doesn't match - it's not
> the name of a flag, and it's not the arg either.
oh, I was confused by these terms :(
> No, again we don't strip flag args.
> (It's not clear how useful/easy to use this would be, maybe we can re-examine
> later?)
we can still strip it by passing "-Werror-unused" arg, right? if so, I think it
should be fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall updated this revision to Diff 277728.
sammccall added a comment.
Add more tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,166 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete args if we've matched the target option.
+ EXPECT_EQ(strip("-I", "clang -Xclang"), "clang -Xclang");
+ EXPECT_EQ(strip("-I", "clang -Xclang -I"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I -Xclang"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I"), "clang");
+}
+
+TEST(ArgStripperTest, Multiple) {
+ ArgStripper S;
+ S.strip("-o");
+ S.strip("-c");
+ std::vector Args = {"clang", "-o", "foo.o", "foo.cc", "-c"};
+ S.process(Args);
+
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall marked an inline comment as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > add tests for stripping the diagnostic flags, `-Wfoo` etc.
> > Those aren't actually separate flags: "-W" is a Joined flag and foo is an
> > arg.
> > Do you want a test specifically for such a string anyway?
> > Or do you want special behavior for them? (Like interaction between -Wfoo
> > and -Wno-foo)
> I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> would be nice to have them in tests as I think these are important cases.
>
> > Those aren't actually separate flags: "-W" is a Joined flag and foo is an
> > arg.
>
> oh, if I understand correctly:
>
> - `strip("unused", "clang -Wunused foo.cc")` => `clang foo.cc` ?
> - `strip("-W", "clang -Wunused -Wextra foo.cc")` => `clang foo.cc` // remove
> all -W flags ?
> - `strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> foo.cc")` => `clang -Wunused -Werror=date-time foo.cc`?
>
> I was a bit unclear about how the stripper strips the "-W/-D"-like flag,
> would be nice to have them in tests as I think these are important cases.
Added.
> strip("unused", "clang -Wunused foo.cc") => clang foo.cc ?
No, we only strip clang args or flag names, not flag args (confusing
terminology).
-W will strip -Wunused (flag name). -Wunused will strip -Wunused (clang arg).
-W* will strip -Wunused (clang arg). but "unused" doesn't match - it's not the
name of a flag, and it's not the arg either.
> strip("-W", "clang -Wunused -Wextra foo.cc") => clang foo.cc // remove all -W
> flags ?
Yes
> strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
> foo.cc") => clang -Wunused -Werror=date-time foo.cc?
No, again we don't strip flag args.
(It's not clear how useful/easy to use this would be, maybe we can re-examine
later?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
looks almost good.
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
sammccall wrote:
> hokein wrote:
> > add tests for stripping the diagnostic flags, `-Wfoo` etc.
> Those aren't actually separate flags: "-W" is a Joined flag and foo is an arg.
> Do you want a test specifically for such a string anyway?
> Or do you want special behavior for them? (Like interaction between -Wfoo and
> -Wno-foo)
I was a bit unclear about how the stripper strips the "-W/-D"-like flag, would
be nice to have them in tests as I think these are important cases.
> Those aren't actually separate flags: "-W" is a Joined flag and foo is an arg.
oh, if I understand correctly:
- `strip("unused", "clang -Wunused foo.cc")` => `clang foo.cc` ?
- `strip("-W", "clang -Wunused -Wextra foo.cc")` => `clang foo.cc` // remove
all -W flags ?
- `strip("error=unused", "clang -Wunused -Werror=unused -Werror=date-time
foo.cc")` => `clang -Wunused -Werror=date-time foo.cc`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall updated this revision to Diff 277508.
sammccall added a comment.
Sigh, last upload was supposed to be a new patch, not clobber this one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,120 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete args if we've matched the target option.
+ EXPECT_EQ(strip("-I", "clang -Xclang"), "clang -Xclang");
+ EXPECT_EQ(strip("-I", "clang -Xclang -I"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I -Xclang"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I"), "clang");
+}
+
+TEST(ArgStripperTest, Multiple) {
+ ArgStripper S;
+ S.strip("-o");
+ S.strip("-c");
+ std::vector Args = {"clang", "-o",
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall updated this revision to Diff 277507.
sammccall added a comment.
[clangd] Config: CompileFlags.Remove
While here, add documentation to CompileFlags and CompileFlags.Add.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -91,10 +91,12 @@
TEST_F(ConfigCompileTests, CompileCommands) {
Frag.CompileFlags.Add.emplace_back("-foo");
- std::vector Argv = {"clang", "a.cc"};
+ Frag.CompileFlags.Remove.emplace_back("--include-directory=");
+ std::vector Argv = {"clang", "-I", "bar/", "a.cc"};
EXPECT_TRUE(compileAndApply());
- EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(1));
- Conf.CompileFlags.Edits.front()(Argv);
+ EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(2));
+ for (auto : Conf.CompileFlags.Edits)
+Edit(Argv);
EXPECT_THAT(Argv, ElementsAre("clang", "a.cc", "-foo"));
}
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,120 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall marked 2 inline comments as done. sammccall added a comment. OK, I think this is good to go now. Comment at: clang-tools-extra/clangd/CompileCommands.cpp:431 +continue; // current arg doesn't match the prefix string + bool PrefixMatch = Arg.size() > R.Text.size(); + unsigned ArgCount = PrefixMatch ? R.PrefixArgs : R.ExactArgs; sammccall wrote: > adamcz wrote: > > Correct me if I'm wrong, but this is relying on the fact that Rules are > > sorted, right? Or, to be more specific, on the fact that -foo comes before > > -foobar. > > > > Consider two rules in config file, one to remove -include, another to > > remove -include-pch, in that order. -include will do a prefix match on Arg > > == -include-pch and attempt to remove exactly one arg (-include is > > JoinedOrSeparate, which has 1 for PrefixArgs), when in reality that was > > "--include-pch foo.pch", an exact match on a different option. > > > > So a config like: > > Remove: [-include, -include-pch] > > and command line: > > [-include-pch, foo.pch] > > should be [], but ends up being [foo.pch] > > > > It looks like Options.inc is sorted in the correct way (include-pch will > > always be before -include). I don't know if that's guaranteed, but it looks > > to be the case. However, since you are adding these options to Rules on > > each strip() call, you end up depending on the order of strip() calls. That > > seems like a bug. > This is a really good point, I hadn't realized the option table was > order-dependent (e.g. I figured -include wasn't a Joined option). > > The real option parser processes the options in the order they appear in the > file, so that should definitely be correct. > I think processing them longest-to-shortest is probably also correct, since a > spelling that's always shadowed by a prefix isn't that useful, I'd hope they > don't actually exist. Opted for explicitly recording the index and traversing all the rules to find the best one (instead of stopping when we find a single match). This should cost a single factor of 2 and it's really simple. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81958/new/ https://reviews.llvm.org/D81958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall updated this revision to Diff 277464.
sammccall added a comment.
Use explicit priority to resolve ambiguities between prefixes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,120 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete args if we've matched the target option.
+ EXPECT_EQ(strip("-I", "clang -Xclang"), "clang -Xclang");
+ EXPECT_EQ(strip("-I", "clang -Xclang -I"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I -Xclang"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I"), "clang");
+}
+
+TEST(ArgStripperTest, Multiple) {
+ ArgStripper S;
+ S.strip("-o");
+ S.strip("-c");
+ std::vector Args = {"clang", "-o", "foo.o",
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall planned changes to this revision.
sammccall added a comment.
Need to work out how to address the order-dependency comment.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:431
+continue; // current arg doesn't match the prefix string
+ bool PrefixMatch = Arg.size() > R.Text.size();
+ unsigned ArgCount = PrefixMatch ? R.PrefixArgs : R.ExactArgs;
adamcz wrote:
> Correct me if I'm wrong, but this is relying on the fact that Rules are
> sorted, right? Or, to be more specific, on the fact that -foo comes before
> -foobar.
>
> Consider two rules in config file, one to remove -include, another to remove
> -include-pch, in that order. -include will do a prefix match on Arg ==
> -include-pch and attempt to remove exactly one arg (-include is
> JoinedOrSeparate, which has 1 for PrefixArgs), when in reality that was
> "--include-pch foo.pch", an exact match on a different option.
>
> So a config like:
> Remove: [-include, -include-pch]
> and command line:
> [-include-pch, foo.pch]
> should be [], but ends up being [foo.pch]
>
> It looks like Options.inc is sorted in the correct way (include-pch will
> always be before -include). I don't know if that's guaranteed, but it looks
> to be the case. However, since you are adding these options to Rules on each
> strip() call, you end up depending on the order of strip() calls. That seems
> like a bug.
This is a really good point, I hadn't realized the option table was
order-dependent (e.g. I figured -include wasn't a Joined option).
The real option parser processes the options in the order they appear in the
file, so that should definitely be correct.
I think processing them longest-to-shortest is probably also correct, since a
spelling that's always shadowed by a prefix isn't that useful, I'd hope they
don't actually exist.
Comment at: clang-tools-extra/clangd/CompileCommands.h:60
+// Args that are not recognized as flags are still removed as literal strings,
+// and strip("ABC*") will remove any arg with an ABC prefix.
+//
hokein wrote:
> Is the glob `*` supported when the Arg (in `strip` API) is recognized as an
> option flag? My reading of the code implies it is not.
>
> Maybe consider moving the above two lines to `strip` API comment, it is more
> discoverable.
Yeah you're right, thought I'd phrase more as "-foo*" is never a recognized
flag :-)
I've moved the comment as you suggest and regrouped to be clearer.
Comment at: clang-tools-extra/clangd/CompileCommands.h:72
+ // Remove the targets from Args, in-place.
+ void process(std::vector ) const;
+
hokein wrote:
> The `Args` is expected to be a standard compile command? if so, might be name
> it `CompileCommand`.
We have this annoying clang-tidy check that wants the impl and decl to have the
same arg names, and CompileCommand is too long for the impl.
Updated the doc though.
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
hokein wrote:
> add tests for stripping the diagnostic flags, `-Wfoo` etc.
Those aren't actually separate flags: "-W" is a Joined flag and foo is an arg.
Do you want a test specifically for such a string anyway?
Or do you want special behavior for them? (Like interaction between -Wfoo and
-Wno-foo)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall updated this revision to Diff 277444.
sammccall marked 13 inline comments as done.
sammccall added a comment.
Address the easy comments, add a test for order-dependency.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -207,6 +207,120 @@
EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello", "-fsyntax-only"));
}
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete args if we've matched the target option.
+ EXPECT_EQ(strip("-I", "clang -Xclang"), "clang -Xclang");
+ EXPECT_EQ(strip("-I", "clang -Xclang -I"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I -Xclang"), "clang");
+ EXPECT_EQ(strip("-I", "clang -I"), "clang");
+}
+
+TEST(ArgStripperTest, Multiple) {
+ ArgStripper S;
+ S.strip("-o");
+ S.strip("-c");
+
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
hokein added inline comments.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:263
+// Flag-parsing mode, which affects which flags are available.
+enum DriverMode : unsigned char {
+ DM_None = 0,
nit: put it into anonymous namespace to avoid potential ODR violation.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:294
+ // We compute a table containing strings to look for and #args to skip.
+ // e.g. "-x" => {-x 2 args, -x* 2 args, --language 2 args, --language=* 1
arg}
+ using TableTy =
Is `-x* 2 args` right? it seems that `-xc++` is just 1 arg.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:329
+// Every spelling of this option will get the same set of rules.
+for (unsigned ID = 1; ID < DriverID::LastOption; ++ID) {
+ if (PrevAlias[ID] || ID == DriverID::OPT_Xclang)
nit: add a comment after 1 e.g `/*skip the OPT_INVALID*/`, explaining why not
starting from 0.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:425
+llvm::StringRef Arg = Args[Read];
+for (const Rule : Rules) {
+ // Rule can fail to match if...
this loop is long and nested within a tricky while, maybe consider pulling it
out a separate function like `getMatchedRule`.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:438
+ if (WasXclang) {
+--Write; // Drop previous -Xclang arg
+CurrentMode = MainMode;
nit: assert(Write > 0)?
Comment at: clang-tools-extra/clangd/CompileCommands.h:60
+// Args that are not recognized as flags are still removed as literal strings,
+// and strip("ABC*") will remove any arg with an ABC prefix.
+//
Is the glob `*` supported when the Arg (in `strip` API) is recognized as an
option flag? My reading of the code implies it is not.
Maybe consider moving the above two lines to `strip` API comment, it is more
discoverable.
Comment at: clang-tools-extra/clangd/CompileCommands.h:72
+ // Remove the targets from Args, in-place.
+ void process(std::vector ) const;
+
The `Args` is expected to be a standard compile command? if so, might be name
it `CompileCommand`.
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
add tests for stripping the diagnostic flags, `-Wfoo` etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
adamcz added inline comments.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:256
+ case Option::RemainingArgsClass:
+return {1, 0};
+ case Option::RemainingArgsJoinedClass:
nit: could you replace 1 with some constant value with descriptive name?
It's not immediately clear if this is some significant value (e.g. some flag
number or something) or just, as it's the case, just a very large number.
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:301
+
+// Collect sets of aliases, so we can treet -foo and -foo= as synonyms.
+// Conceptually a double-linked list: PrevAlias[I] -> I -> NextAlias[I].
typo: treet
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:401
+
+ // Examine args list to determine if we're in GCC, CL-compatible, or cc1
mode.
+ DriverMode MainMode = DM_GCC;
nit: this function is already quite long and this part seems like a re-usable
and self-contained piece. Could we extra this into some GetDriverMode() helper?
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:431
+continue; // current arg doesn't match the prefix string
+ bool PrefixMatch = Arg.size() > R.Text.size();
+ unsigned ArgCount = PrefixMatch ? R.PrefixArgs : R.ExactArgs;
Correct me if I'm wrong, but this is relying on the fact that Rules are sorted,
right? Or, to be more specific, on the fact that -foo comes before -foobar.
Consider two rules in config file, one to remove -include, another to remove
-include-pch, in that order. -include will do a prefix match on Arg ==
-include-pch and attempt to remove exactly one arg (-include is
JoinedOrSeparate, which has 1 for PrefixArgs), when in reality that was
"--include-pch foo.pch", an exact match on a different option.
So a config like:
Remove: [-include, -include-pch]
and command line:
[-include-pch, foo.pch]
should be [], but ends up being [foo.pch]
It looks like Options.inc is sorted in the correct way (include-pch will always
be before -include). I don't know if that's guaranteed, but it looks to be the
case. However, since you are adding these options to Rules on each strip()
call, you end up depending on the order of strip() calls. That seems like a bug.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81958/new/
https://reviews.llvm.org/D81958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D81958: [clangd] Add library to semantically strip flags by name.
sammccall created this revision.
sammccall added reviewers: adamcz, hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
This is designed for tweaking compile commands by specifying flags to add/remove
in a config file. Something like:
CompileFlags: { Remove: -fcolor-diagnostics }
Having users tweak raw argv (e.g. with a regex) is going to end in tears: bugs
around clang-cl, xclang, aliases, joined-vs-separate args etc are inevitable.
This isn't in tooling because of the performance choices: build a big table
up-front to make subsequent actions fast. Maybe it should be though.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81958
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -185,6 +185,108 @@
}
#endif
+static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
+ llvm::SmallVector Parts;
+ llvm::SplitString(Argv, Parts);
+ std::vector Args = {Parts.begin(), Parts.end()};
+ ArgStripper S;
+ S.strip(Arg);
+ S.process(Args);
+ return llvm::join(Args, " ");
+}
+
+TEST(ArgStripperTest, Spellings) {
+ // May use alternate prefixes.
+ EXPECT_EQ(strip("-pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang -pedantic foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--pedantic", "clang --pedantic foo.cc"), "clang foo.cc");
+ // May use alternate names.
+ EXPECT_EQ(strip("-x", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-x", "clang --language=c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang -x c++ foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("--language=", "clang --language=c++ foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, UnknownFlag) {
+ EXPECT_EQ(strip("-xyzzy", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyz*", "clang -xyzzy foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-xyzzy", "clang -Xclang -xyzzy foo.cc"), "clang foo.cc");
+}
+
+TEST(ArgStripperTest, Xclang) {
+ // Flags may be -Xclang escaped.
+ EXPECT_EQ(strip("-ast-dump", "clang -Xclang -ast-dump foo.cc"),
+"clang foo.cc");
+ // Args may be -Xclang escaped.
+ EXPECT_EQ(strip("-add-plugin", "clang -Xclang -add-plugin -Xclang z foo.cc"),
+"clang foo.cc");
+}
+
+TEST(ArgStripperTest, ClangCL) {
+ // /I is a synonym for -I in clang-cl mode only.
+ // Not stripped by default.
+ EXPECT_EQ(strip("-I", "clang -I /usr/inc /Interesting/file.cc"),
+"clang /Interesting/file.cc");
+ // Stripped when invoked as clang-cl.
+ EXPECT_EQ(strip("-I", "clang-cl -I /usr/inc /Interesting/file.cc"),
+"clang-cl");
+ // Stripped when invoked as CL.EXE
+ EXPECT_EQ(strip("-I", "CL.EXE -I /usr/inc /Interesting/file.cc"), "CL.EXE");
+ // Stripped when passed --driver-mode=cl.
+ EXPECT_EQ(strip("-I", "cc -I /usr/inc /Interesting/file.cc --driver-mode=cl"),
+"cc --driver-mode=cl");
+}
+
+TEST(ArgStripperTest, ArgStyles) {
+ // Flag
+ EXPECT_EQ(strip("-Qn", "clang -Qn foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Qn", "clang -QnZ foo.cc"), "clang -QnZ foo.cc");
+ // Joined
+ EXPECT_EQ(strip("-std=", "clang -std= foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-std=", "clang -std=c++11 foo.cc"), "clang foo.cc");
+ // Separate
+ EXPECT_EQ(strip("-mllvm", "clang -mllvm X foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-mllvm", "clang -mllvmX foo.cc"), "clang -mllvmX foo.cc");
+ // RemainingArgsJoined
+ EXPECT_EQ(strip("/link", "clang-cl /link b c d foo.cc"), "clang-cl");
+ EXPECT_EQ(strip("/link", "clang-cl /linka b c d foo.cc"), "clang-cl");
+ // CommaJoined
+ EXPECT_EQ(strip("-Wl,", "clang -Wl,x,y foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-Wl,", "clang -Wl, foo.cc"), "clang foo.cc");
+ // MultiArg
+ EXPECT_EQ(strip("-segaddr", "clang -segaddr a b foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-segaddr", "clang -segaddra b foo.cc"),
+"clang -segaddra b foo.cc");
+ // JoinedOrSeparate
+ EXPECT_EQ(strip("-G", "clang -GX foo.cc"), "clang foo.cc");
+ EXPECT_EQ(strip("-G", "clang -G X foo.cc"), "clang foo.cc");
+ // JoinedAndSeparate
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg-X Y foo.cc"),
+"clang -cc1 foo.cc");
+ EXPECT_EQ(strip("-plugin-arg-", "clang -cc1 -plugin-arg- Y foo.cc"),
+"clang -cc1 foo.cc");
+}
+
+TEST(ArgStripperTest, EndOfList) {
+ // When we hit the end-of-args prematurely, we don't crash.
+ // We consume the incomplete
