[PATCH] D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 15 11:34:55 PST 2019
aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:30
+ // They appear in the AST just *prior* to the typedefs.
+ Finder->addMatcher(cxxRecordDecl().bind("struct"), this);
}
----------------
It's unfortunate that we can't restrict this matcher more -- there tend to be a lot of struct declarations, so I am a bit worried about the performance of this matching on so many of them. At a minimum, I think you can restrict it to non-implicit structs here and simplify the code in `check()` a bit.
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:55
- // do not fix if there is macro or array
- if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID())
+ // warn at StartLoc but do not fix if there is macro or array
+ if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
----------------
warn -> Warn
Add a full stop to the end of the comment.
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:57
+ if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
+ auto Diag = diag(StartLoc, UseUsingWarning);
return;
----------------
Is there a purpose to the `Diag` local variable, or can it just be removed?
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:85
+ ReplaceRange.setBegin(LastReplacementEnd);
+ Using = "; using ";
+
----------------
Should there be a newline here, to avoid putting all the using declarations on the same line?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70270/new/
https://reviews.llvm.org/D70270
More information about the cfe-commits
mailing list