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
