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

[Sam McCall via Phabricator via cfe-commits]

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<std::string> &Args) 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