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
