[PATCH] Add readability-remove-void-arg check to clang-tidy

Alexander Kornienko alexfh at google.com
Sun Feb 22 18:45:34 PST 2015


Sorry for the long delay. Here are some initial comments. It will be easier to review once you reformat the code and place consts in front (or remove, if needed) to make the code closer to the LLVM style.


================
Comment at: clang-tidy/readability/RemoveVoidArg.cpp:170
@@ +169,3 @@
+    std::string const Text = getText(Result, *Typedef);
+    removeVoidArgumentTokens(Result, Typedef->getLocStart(), Text, "Redundant void argument list in typedef.");
+}
----------------
1. The "Redundant void argument list in" part is repeated everywhere. Maybe just pass the description of the location (here - "typedef") as an argument to `removeVoidArgumentTokens`?

2. Clang-tidy messages should use the same style as Clang diagnostics: start with a lower-case letter, and no trailing period.

================
Comment at: clang-tidy/readability/RemoveVoidArg.h:26
@@ +25,3 @@
+///   int foo(void); ==> int foo();
+class RemoveVoidArg : public ClangTidyCheck
+{
----------------
Formatting is off in many places. Could you clang-format the code so I don't need to point to every nit?

================
Comment at: clang-tidy/readability/RemoveVoidArg.h:38
@@ +37,3 @@
+private:
+    void processFunctionDecl(const ast_matchers::MatchFinder::MatchResult &Result, FunctionDecl const *const Function);
+
----------------
Here and below: I'm against top-level `const` in function parameter types. It's just an implementation detail whether the method can modify its parameter. It doesn't make sense to pollute the interface with this.

Also, it's more common in Clang/LLVM code to put the const related to the pointed/referenced object in front:

  void processFunctionDecl(const ast_matchers::MatchFinder::MatchResult &Result, const FunctionDecl *Function);

================
Comment at: test/clang-tidy/readability-remove-void-arg.cpp:28
@@ +27,3 @@
+int (*returns_fn_void_int(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: Redundant void argument list in function declaration. [readability-remove-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: Redundant void argument list in function declaration. [readability-remove-void-arg]
----------------
Repeating the whole message each time makes the test harder to read. Please leave the whole message once and shorten all other occurrences, e.g.:

  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function declaration

================
Comment at: test/clang-tidy/readability-remove-void-arg.cpp:145
@@ +144,3 @@
+// CHECK-FIXES: {{^}}typedef void (function_ptr2){{$}}
+// CHECK-FIXES-NEXT: {{^    }}({{$}}
+// CHECK-FIXES-NEXT: {{^        $}}
----------------
It would be more readable with a single regular expression:
  {{^    \($}}

http://reviews.llvm.org/D7639

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list