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