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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
