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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:263
+// Flag-parsing mode, which affects which flags are available.
+enum DriverMode : unsigned char {
+  DM_None = 0,
----------------
nit: put it into anonymous namespace to avoid potential ODR violation.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:294
+  // We compute a table containing strings to look for and #args to skip.
+  // e.g. "-x" => {-x 2 args, -x* 2 args, --language 2 args, --language=* 1 arg}
+  using TableTy =
----------------
Is `-x* 2 args` right? it seems that `-xc++` is just 1 arg.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:329
+    // Every spelling of this option will get the same set of rules.
+    for (unsigned ID = 1; ID < DriverID::LastOption; ++ID) {
+      if (PrevAlias[ID] || ID == DriverID::OPT_Xclang)
----------------
nit: add a comment after 1 e.g `/*skip the OPT_INVALID*/`, explaining why not starting from 0.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:425
+    llvm::StringRef Arg = Args[Read];
+    for (const Rule &R : Rules) {
+      // Rule can fail to match if...
----------------
this loop is long and nested within a tricky while, maybe consider pulling it out a separate function like `getMatchedRule`.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:438
+      if (WasXclang) {
+        --Write; // Drop previous -Xclang arg
+        CurrentMode = MainMode;
----------------
nit: assert(Write > 0)?


================
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.
+//
----------------
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.


================
Comment at: clang-tools-extra/clangd/CompileCommands.h:72
+  // Remove the targets from Args, in-place.
+  void process(std::vector<std::string> &Args) const;
+
----------------
The `Args` is expected to be a standard compile command? if so, might be name it `CompileCommand`.


================
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:197
+}
+
+TEST(ArgStripperTest, Spellings) {
----------------
add tests for stripping the diagnostic flags, `-Wfoo` etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81958