[PATCH] D81958: [clangd] Add library to semantically strip flags by name.

[Sam McCall via Phabricator via cfe-commits]

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