[PATCH] D91789: [clang-tidy] find/fix unneeded trailing semicolons in macros

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp:49
+
+void ExtraSemiCheck::registerMatchers(MatchFinder *Finder) {
+  if (FixerKind == ESFK_Switch) {
----------------
Something that's not for you to solve, but for us to think about...

The AST matchers have ways to inspect parent and child relationships between AST nodes but no way to inspect sibling relationships. It's really unfortunate just how much effort you have to go through after the AST matcher fires to figure this out. It would be much cleaner if there was a way for you to do something like: `compoundStmt(hasSiblings(switchStmt(), nullStmt().bind("null")))` to test for a `SwitchStmt` node immediately followed by a `NullStmt` node that appear inside a `CompoundStmt`. I feel like that would make these checks a lot easier for you to write.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h:22
+
+  enum ExtraSemiFixerKind { ESFK_None, ESFK_Switch, ESFK_TrailingMacro };
+
----------------
I'm not familiar with the way your CI plans to use this check, but: would it make sense to treat these as a bitmask rather than mutually exclusive options? e.g., the CI can enable both "switch" and "trailing macro" in a single pass of a file rather than run the check twice with different options on the same file? The configuration could then take a delimited list of pass options to enable to set up this bitmask, and the `==` tests for the `FixerKind` would then become `&` tests.


================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h:37
+  std::vector<const MacroInfo *> SuspectMacros;
+  enum ExtraSemiFixerKind FixerKind;
+  const std::string ExtraSemiFixerKindName;
----------------
You can drop the `enum` elaboration here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91789