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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:256
+  case Option::RemainingArgsClass:
+    return {10000, 0};
+  case Option::RemainingArgsJoinedClass:
----------------
nit: could you replace 10000 with some constant value with descriptive name?  It's not immediately clear if this is some significant value (e.g. some flag number or something) or just, as it's the case, just a very large number.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:301
+
+    // Collect sets of aliases, so we can treet -foo and -foo= as synonyms.
+    // Conceptually a double-linked list: PrevAlias[I] -> I -> NextAlias[I].
----------------
typo: treet


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:401
+
+  // Examine args list to determine if we're in GCC, CL-compatible, or cc1 mode.
+  DriverMode MainMode = DM_GCC;
----------------
nit: this function is already quite long and this part seems like a re-usable and self-contained piece. Could we extra this into some GetDriverMode() helper?


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81958/new/

https://reviews.llvm.org/D81958