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