[PATCH] D51949: [clang-tidy] new check 'readability-isolate-declaration'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you add some tests for cases like:

  // C code
  void (*signal(int sig, void (*func)(int)))(int); // One decl
  int i = sizeof(struct S { int i;}); // One decl
  int j = sizeof(struct T { int i;}), k; // Two decls
  void g(struct U { int i; } s); // One decl
  void h(struct V { int i; } s), m(int i, ...); // Two decls

and

  // C++ code
  void f() {
    switch (int i = 12, j = 14; i) {} // Two decls, but don't transform
  }
  
  void g() try {
    int i, j; // Two decls
  } catch (...) {}



================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:24
+AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); }
+AST_MATCHER(DeclStmt, isVariablesOnly) {
+  return std::all_of(Node.decl_begin(), Node.decl_end(),
----------------
The identifier reads strangely to my ears. How about: `onlyDeclaresVariables()`?


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:52
+                                             const LangOptions &LangOpts) {
+  assert(Indirections >= 0 && "Indirections must be non-negative");
+  if (Indirections == 0)
----------------
This assertion suggests that `Indirections` should be `unsigned` and that you perform the assertion before doing a decrement to ensure it isn't trying to decrement past 0.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:76-78
+    const auto *Pointee =
+        T->getPointeeType().getTypePtr()->getAs<FunctionType>();
+    assert(Pointee && "Expected FunctionType Pointer");
----------------
Use `castAs<FunctionType>()` and remove the assert.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:83-84
+
+  if (isa<ArrayType>(T)) {
+    const auto *AT = cast<ArrayType>(T);
+    // Note: Do not increment the 'Indirections' because it is not yet clear
----------------
`if (const auto *AT = dyn_cast<ArrayType>(T))` rather than isa followed by cast.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:150
+
+  // FIXME: Memberpointer are not transformed correctly right now, thats
+  // why they are treated as problematic here.
----------------
Memberpointer -> Member pointer
thats -> that's

(Same elsewhere)


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:199-200
+  for (auto It = DS->decl_begin(), End = DS->decl_end(); It != End; ++It) {
+    VarDecl *CurrentDecl = dyn_cast<VarDecl>(*It);
+    assert(CurrentDecl && "Expect only VarDecls here");
+
----------------
Use `cast<>` instead of `dyn_cast` and an assert.


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:230
+  for (const auto &Range : Ranges) {
+    auto CharRange = Lexer::getAsCharRange(
+        CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
----------------
Use of `auto`?


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:250
+
+/// Excepts a vector {TypeSnippet, Firstdecl, SecondDecl, ...}.
+static std::vector<std::string>
----------------
Excepts -> Accepts


================
Comment at: clang-tidy/readability/IsolateDeclarationCheck.cpp:291
+          .str());
+  // llvm::dbgs() << Replacement << "\n";
+
----------------
Debugging code that can be removed.


================
Comment at: clang-tidy/utils/LexerUtils.cpp:85
+    llvm::Optional<Token> Tok = Lexer::findNextToken(Loc, SM, LangOpts);
+    assert(Tok && "Could not retrieve next token");
+
----------------
This seems like a bad assertion -- it's an optional for a reason, isn't it?


================
Comment at: clang-tidy/utils/LexerUtils.h:48
+    Token T;
+    bool FailedRetrievingToken = Lexer::getRawToken(L, T, SM, LangOpts);
+
----------------
No real need for this local.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949