[clang-tidy] Code cleanup, (almost) NFC (*).

(*) Printed types of member pointers don't use elaborated type specifiers
(`int struct S::*` -> `int S::*`).

llvm-svn: 302160
This commit is contained in:
Alexander Kornienko 2017-05-04 15:34:23 +00:00
parent bde6fa9ca7
commit cbe8d16da4
4 changed files with 95 additions and 152 deletions

View File

@ -11,6 +11,7 @@
#include "clang/AST/ASTContext.h" #include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h" #include "clang/Lex/Lexer.h"
#include "clang/Tooling/FixIt.h"
#include <queue> #include <queue>
using namespace clang::ast_matchers; using namespace clang::ast_matchers;
@ -32,7 +33,7 @@ bool isNULLMacroExpansion(const Stmt *Statement, ASTContext &Context) {
const LangOptions &LO = Context.getLangOpts(); const LangOptions &LO = Context.getLangOpts();
SourceLocation Loc = Statement->getLocStart(); SourceLocation Loc = Statement->getLocStart();
return SM.isMacroBodyExpansion(Loc) && return SM.isMacroBodyExpansion(Loc) &&
clang::Lexer::getImmediateMacroName(Loc, SM, LO) == "NULL"; Lexer::getImmediateMacroName(Loc, SM, LO) == "NULL";
} }
AST_MATCHER(Stmt, isNULLMacroExpansion) { AST_MATCHER(Stmt, isNULLMacroExpansion) {
@ -57,17 +58,15 @@ StatementMatcher createImplicitCastFromBoolMatcher() {
hasSourceExpression(expr(hasType(qualType(booleanType()))))); hasSourceExpression(expr(hasType(qualType(booleanType())))));
} }
StringRef StringRef getZeroLiteralToCompareWithForType(CastKind CastExprKind,
getZeroLiteralToCompareWithForGivenType(CastKind CastExpressionKind, QualType Type,
QualType CastSubExpressionType,
ASTContext &Context) { ASTContext &Context) {
switch (CastExpressionKind) { switch (CastExprKind) {
case CK_IntegralToBoolean: case CK_IntegralToBoolean:
return CastSubExpressionType->isUnsignedIntegerType() ? "0u" : "0"; return Type->isUnsignedIntegerType() ? "0u" : "0";
case CK_FloatingToBoolean: case CK_FloatingToBoolean:
return Context.hasSameType(CastSubExpressionType, Context.FloatTy) ? "0.0f" return Context.hasSameType(Type, Context.FloatTy) ? "0.0f" : "0.0";
: "0.0";
case CK_PointerToBoolean: case CK_PointerToBoolean:
case CK_MemberPointerToBoolean: // Fall-through on purpose. case CK_MemberPointerToBoolean: // Fall-through on purpose.
@ -79,10 +78,8 @@ getZeroLiteralToCompareWithForGivenType(CastKind CastExpressionKind,
} }
bool isUnaryLogicalNotOperator(const Stmt *Statement) { bool isUnaryLogicalNotOperator(const Stmt *Statement) {
const auto *UnaryOperatorExpression = const auto *UnaryOperatorExpr = dyn_cast<UnaryOperator>(Statement);
llvm::dyn_cast<UnaryOperator>(Statement); return UnaryOperatorExpr && UnaryOperatorExpr->getOpcode() == UO_LNot;
return UnaryOperatorExpression != nullptr &&
UnaryOperatorExpression->getOpcode() == UO_LNot;
} }
bool areParensNeededForOverloadedOperator(OverloadedOperatorKind OperatorKind) { bool areParensNeededForOverloadedOperator(OverloadedOperatorKind OperatorKind) {
@ -103,39 +100,35 @@ bool areParensNeededForOverloadedOperator(OverloadedOperatorKind OperatorKind) {
} }
bool areParensNeededForStatement(const Stmt *Statement) { bool areParensNeededForStatement(const Stmt *Statement) {
if (const auto *OverloadedOperatorCall = if (const auto *OperatorCall = dyn_cast<CXXOperatorCallExpr>(Statement)) {
llvm::dyn_cast<CXXOperatorCallExpr>(Statement)) { return areParensNeededForOverloadedOperator(OperatorCall->getOperator());
return areParensNeededForOverloadedOperator(
OverloadedOperatorCall->getOperator());
} }
return llvm::isa<BinaryOperator>(Statement) || return isa<BinaryOperator>(Statement) || isa<UnaryOperator>(Statement);
llvm::isa<UnaryOperator>(Statement);
} }
void addFixItHintsForGenericExpressionCastToBool( void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
DiagnosticBuilder &Diagnostic, const ImplicitCastExpr *CastExpression, const ImplicitCastExpr *Cast, const Stmt *Parent,
const Stmt *ParentStatement, ASTContext &Context) { ASTContext &Context) {
// In case of expressions like (! integer), we should remove the redundant not // In case of expressions like (! integer), we should remove the redundant not
// operator and use inverted comparison (integer == 0). // operator and use inverted comparison (integer == 0).
bool InvertComparison = bool InvertComparison =
ParentStatement != nullptr && isUnaryLogicalNotOperator(ParentStatement); Parent != nullptr && isUnaryLogicalNotOperator(Parent);
if (InvertComparison) { if (InvertComparison) {
SourceLocation ParentStartLoc = ParentStatement->getLocStart(); SourceLocation ParentStartLoc = Parent->getLocStart();
SourceLocation ParentEndLoc = SourceLocation ParentEndLoc =
llvm::cast<UnaryOperator>(ParentStatement)->getSubExpr()->getLocStart(); cast<UnaryOperator>(Parent)->getSubExpr()->getLocStart();
Diagnostic.AddFixItHint(FixItHint::CreateRemoval( Diag << FixItHint::CreateRemoval(
CharSourceRange::getCharRange(ParentStartLoc, ParentEndLoc))); CharSourceRange::getCharRange(ParentStartLoc, ParentEndLoc));
auto FurtherParents = Context.getParents(*ParentStatement); Parent = Context.getParents(*Parent)[0].get<Stmt>();
ParentStatement = FurtherParents[0].get<Stmt>();
} }
const Expr *SubExpression = CastExpression->getSubExpr(); const Expr *SubExpr = Cast->getSubExpr();
bool NeedInnerParens = areParensNeededForStatement(SubExpression); bool NeedInnerParens = areParensNeededForStatement(SubExpr);
bool NeedOuterParens = ParentStatement != nullptr && bool NeedOuterParens =
areParensNeededForStatement(ParentStatement); Parent != nullptr && areParensNeededForStatement(Parent);
std::string StartLocInsertion; std::string StartLocInsertion;
@ -147,9 +140,7 @@ void addFixItHintsForGenericExpressionCastToBool(
} }
if (!StartLocInsertion.empty()) { if (!StartLocInsertion.empty()) {
SourceLocation StartLoc = CastExpression->getLocStart(); Diag << FixItHint::CreateInsertion(Cast->getLocStart(), StartLocInsertion);
Diagnostic.AddFixItHint(
FixItHint::CreateInsertion(StartLoc, StartLocInsertion));
} }
std::string EndLocInsertion; std::string EndLocInsertion;
@ -164,128 +155,91 @@ void addFixItHintsForGenericExpressionCastToBool(
EndLocInsertion += " != "; EndLocInsertion += " != ";
} }
EndLocInsertion += getZeroLiteralToCompareWithForGivenType( EndLocInsertion += getZeroLiteralToCompareWithForType(
CastExpression->getCastKind(), SubExpression->getType(), Context); Cast->getCastKind(), SubExpr->getType(), Context);
if (NeedOuterParens) { if (NeedOuterParens) {
EndLocInsertion += ")"; EndLocInsertion += ")";
} }
SourceLocation EndLoc = Lexer::getLocForEndOfToken( SourceLocation EndLoc = Lexer::getLocForEndOfToken(
CastExpression->getLocEnd(), 0, Context.getSourceManager(), Cast->getLocEnd(), 0, Context.getSourceManager(), Context.getLangOpts());
Context.getLangOpts()); Diag << FixItHint::CreateInsertion(EndLoc, EndLocInsertion);
Diagnostic.AddFixItHint(FixItHint::CreateInsertion(EndLoc, EndLocInsertion));
} }
StringRef getEquivalentBoolLiteralForExpression(const Expr *Expression, StringRef getEquivalentBoolLiteralForExpr(const Expr *Expression,
ASTContext &Context) { ASTContext &Context) {
if (isNULLMacroExpansion(Expression, Context)) { if (isNULLMacroExpansion(Expression, Context)) {
return "false"; return "false";
} }
if (const auto *IntLit = llvm::dyn_cast<IntegerLiteral>(Expression)) { if (const auto *IntLit = dyn_cast<IntegerLiteral>(Expression)) {
return (IntLit->getValue() == 0) ? "false" : "true"; return (IntLit->getValue() == 0) ? "false" : "true";
} }
if (const auto *FloatLit = llvm::dyn_cast<FloatingLiteral>(Expression)) { if (const auto *FloatLit = dyn_cast<FloatingLiteral>(Expression)) {
llvm::APFloat FloatLitAbsValue = FloatLit->getValue(); llvm::APFloat FloatLitAbsValue = FloatLit->getValue();
FloatLitAbsValue.clearSign(); FloatLitAbsValue.clearSign();
return (FloatLitAbsValue.bitcastToAPInt() == 0) ? "false" : "true"; return (FloatLitAbsValue.bitcastToAPInt() == 0) ? "false" : "true";
} }
if (const auto *CharLit = llvm::dyn_cast<CharacterLiteral>(Expression)) { if (const auto *CharLit = dyn_cast<CharacterLiteral>(Expression)) {
return (CharLit->getValue() == 0) ? "false" : "true"; return (CharLit->getValue() == 0) ? "false" : "true";
} }
if (llvm::isa<StringLiteral>(Expression->IgnoreCasts())) { if (isa<StringLiteral>(Expression->IgnoreCasts())) {
return "true"; return "true";
} }
return StringRef(); return StringRef();
} }
void addFixItHintsForLiteralCastToBool(DiagnosticBuilder &Diagnostic, void fixGenericExprCastFromBool(DiagnosticBuilder &Diag,
const ImplicitCastExpr *CastExpression, const ImplicitCastExpr *Cast,
StringRef EquivalentLiteralExpression) {
SourceLocation StartLoc = CastExpression->getLocStart();
SourceLocation EndLoc = CastExpression->getLocEnd();
Diagnostic.AddFixItHint(FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(StartLoc, EndLoc),
EquivalentLiteralExpression));
}
void addFixItHintsForGenericExpressionCastFromBool(
DiagnosticBuilder &Diagnostic, const ImplicitCastExpr *CastExpression,
ASTContext &Context, StringRef OtherType) { ASTContext &Context, StringRef OtherType) {
const Expr *SubExpression = CastExpression->getSubExpr(); const Expr *SubExpr = Cast->getSubExpr();
bool NeedParens = !llvm::isa<ParenExpr>(SubExpression); bool NeedParens = !isa<ParenExpr>(SubExpr);
std::string StartLocInsertion = "static_cast<"; Diag << FixItHint::CreateInsertion(
StartLocInsertion += OtherType.str(); Cast->getLocStart(),
StartLocInsertion += ">"; (Twine("static_cast<") + OtherType + ">" + (NeedParens ? "(" : ""))
if (NeedParens) { .str());
StartLocInsertion += "(";
}
SourceLocation StartLoc = CastExpression->getLocStart();
Diagnostic.AddFixItHint(
FixItHint::CreateInsertion(StartLoc, StartLocInsertion));
if (NeedParens) { if (NeedParens) {
SourceLocation EndLoc = Lexer::getLocForEndOfToken( SourceLocation EndLoc = Lexer::getLocForEndOfToken(
CastExpression->getLocEnd(), 0, Context.getSourceManager(), Cast->getLocEnd(), 0, Context.getSourceManager(),
Context.getLangOpts()); Context.getLangOpts());
Diagnostic.AddFixItHint(FixItHint::CreateInsertion(EndLoc, ")")); Diag << FixItHint::CreateInsertion(EndLoc, ")");
} }
} }
StringRef getEquivalentLiteralForBoolLiteral( StringRef getEquivalentForBoolLiteral(const CXXBoolLiteralExpr *BoolLiteral,
const CXXBoolLiteralExpr *BoolLiteralExpression, QualType DestinationType, QualType DestType, ASTContext &Context) {
ASTContext &Context) {
// Prior to C++11, false literal could be implicitly converted to pointer. // Prior to C++11, false literal could be implicitly converted to pointer.
if (!Context.getLangOpts().CPlusPlus11 && if (!Context.getLangOpts().CPlusPlus11 &&
(DestinationType->isPointerType() || (DestType->isPointerType() || DestType->isMemberPointerType()) &&
DestinationType->isMemberPointerType()) && BoolLiteral->getValue() == false) {
BoolLiteralExpression->getValue() == false) {
return "0"; return "0";
} }
if (DestinationType->isFloatingType()) { if (DestType->isFloatingType()) {
if (BoolLiteralExpression->getValue() == true) { if (Context.hasSameType(DestType, Context.FloatTy)) {
return Context.hasSameType(DestinationType, Context.FloatTy) ? "1.0f" return BoolLiteral->getValue() ? "1.0f" : "0.0f";
: "1.0";
} }
return Context.hasSameType(DestinationType, Context.FloatTy) ? "0.0f" return BoolLiteral->getValue() ? "1.0" : "0.0";
: "0.0";
} }
if (BoolLiteralExpression->getValue() == true) { if (DestType->isUnsignedIntegerType()) {
return DestinationType->isUnsignedIntegerType() ? "1u" : "1"; return BoolLiteral->getValue() ? "1u" : "0u";
} }
return DestinationType->isUnsignedIntegerType() ? "0u" : "0"; return BoolLiteral->getValue() ? "1" : "0";
} }
void addFixItHintsForLiteralCastFromBool(DiagnosticBuilder &Diagnostic, bool isAllowedConditionalCast(const ImplicitCastExpr *Cast,
const ImplicitCastExpr *CastExpression,
ASTContext &Context,
QualType DestinationType) {
SourceLocation StartLoc = CastExpression->getLocStart();
SourceLocation EndLoc = CastExpression->getLocEnd();
const auto *BoolLiteralExpression =
llvm::dyn_cast<CXXBoolLiteralExpr>(CastExpression->getSubExpr());
Diagnostic.AddFixItHint(FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(StartLoc, EndLoc),
getEquivalentLiteralForBoolLiteral(BoolLiteralExpression, DestinationType,
Context)));
}
bool isAllowedConditionalCast(const ImplicitCastExpr *CastExpression,
ASTContext &Context) { ASTContext &Context) {
std::queue<const Stmt *> Q; std::queue<const Stmt *> Q;
Q.push(CastExpression); Q.push(Cast);
while (!Q.empty()) { while (!Q.empty()) {
for (const auto &N : Context.getParents(*Q.front())) { for (const auto &N : Context.getParents(*Q.front())) {
const Stmt *S = N.get<Stmt>(); const Stmt *S = N.get<Stmt>();
@ -294,8 +248,7 @@ bool isAllowedConditionalCast(const ImplicitCastExpr *CastExpression,
if (isa<IfStmt>(S) || isa<ConditionalOperator>(S)) if (isa<IfStmt>(S) || isa<ConditionalOperator>(S))
return true; return true;
if (isa<ParenExpr>(S) || isa<ImplicitCastExpr>(S) || if (isa<ParenExpr>(S) || isa<ImplicitCastExpr>(S) ||
(isa<UnaryOperator>(S) && isUnaryLogicalNotOperator(S) ||
cast<UnaryOperator>(S)->getOpcode() == UO_LNot) ||
(isa<BinaryOperator>(S) && cast<BinaryOperator>(S)->isLogicalOp())) { (isa<BinaryOperator>(S) && cast<BinaryOperator>(S)->isLogicalOp())) {
Q.push(S); Q.push(S);
} else { } else {
@ -371,69 +324,59 @@ void ImplicitBoolCastCheck::registerMatchers(MatchFinder *Finder) {
void ImplicitBoolCastCheck::check(const MatchFinder::MatchResult &Result) { void ImplicitBoolCastCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *CastToBool = if (const auto *CastToBool =
Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastToBool")) { Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastToBool")) {
const auto *ParentStatement = Result.Nodes.getNodeAs<Stmt>("parentStmt"); const auto *Parent = Result.Nodes.getNodeAs<Stmt>("parentStmt");
return handleCastToBool(CastToBool, ParentStatement, *Result.Context); return handleCastToBool(CastToBool, Parent, *Result.Context);
} }
if (const auto *CastFromBool = if (const auto *CastFromBool =
Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastFromBool")) { Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastFromBool")) {
const auto *FurtherImplicitCastExpression = const auto *NextImplicitCast =
Result.Nodes.getNodeAs<ImplicitCastExpr>("furtherImplicitCast"); Result.Nodes.getNodeAs<ImplicitCastExpr>("furtherImplicitCast");
return handleCastFromBool(CastFromBool, FurtherImplicitCastExpression, return handleCastFromBool(CastFromBool, NextImplicitCast, *Result.Context);
*Result.Context);
} }
} }
void ImplicitBoolCastCheck::handleCastToBool( void ImplicitBoolCastCheck::handleCastToBool(const ImplicitCastExpr *Cast,
const ImplicitCastExpr *CastExpression, const Stmt *ParentStatement, const Stmt *Parent,
ASTContext &Context) { ASTContext &Context) {
if (AllowConditionalPointerCasts && if (AllowConditionalPointerCasts &&
(CastExpression->getCastKind() == CK_PointerToBoolean || (Cast->getCastKind() == CK_PointerToBoolean ||
CastExpression->getCastKind() == CK_MemberPointerToBoolean) && Cast->getCastKind() == CK_MemberPointerToBoolean) &&
isAllowedConditionalCast(CastExpression, Context)) { isAllowedConditionalCast(Cast, Context)) {
return; return;
} }
if (AllowConditionalIntegerCasts && if (AllowConditionalIntegerCasts &&
CastExpression->getCastKind() == CK_IntegralToBoolean && Cast->getCastKind() == CK_IntegralToBoolean &&
isAllowedConditionalCast(CastExpression, Context)) { isAllowedConditionalCast(Cast, Context)) {
return; return;
} }
std::string OtherType = CastExpression->getSubExpr()->getType().getAsString(); auto Diag = diag(Cast->getLocStart(), "implicit cast %0 -> bool")
DiagnosticBuilder Diagnostic = << Cast->getSubExpr()->getType();
diag(CastExpression->getLocStart(), "implicit cast '%0' -> bool")
<< OtherType;
StringRef EquivalentLiteralExpression = getEquivalentBoolLiteralForExpression( StringRef EquivalentLiteral =
CastExpression->getSubExpr(), Context); getEquivalentBoolLiteralForExpr(Cast->getSubExpr(), Context);
if (!EquivalentLiteralExpression.empty()) { if (!EquivalentLiteral.empty()) {
addFixItHintsForLiteralCastToBool(Diagnostic, CastExpression, Diag << tooling::fixit::createReplacement(*Cast, EquivalentLiteral);
EquivalentLiteralExpression);
} else { } else {
addFixItHintsForGenericExpressionCastToBool(Diagnostic, CastExpression, fixGenericExprCastToBool(Diag, Cast, Parent, Context);
ParentStatement, Context);
} }
} }
void ImplicitBoolCastCheck::handleCastFromBool( void ImplicitBoolCastCheck::handleCastFromBool(
const ImplicitCastExpr *CastExpression, const ImplicitCastExpr *Cast, const ImplicitCastExpr *NextImplicitCast,
const ImplicitCastExpr *FurtherImplicitCastExpression,
ASTContext &Context) { ASTContext &Context) {
QualType DestinationType = (FurtherImplicitCastExpression != nullptr) QualType DestType =
? FurtherImplicitCastExpression->getType() NextImplicitCast ? NextImplicitCast->getType() : Cast->getType();
: CastExpression->getType(); auto Diag = diag(Cast->getLocStart(), "implicit cast bool -> %0") << DestType;
std::string DestinationTypeString = DestinationType.getAsString();
DiagnosticBuilder Diagnostic =
diag(CastExpression->getLocStart(), "implicit cast bool -> '%0'")
<< DestinationTypeString;
if (llvm::isa<CXXBoolLiteralExpr>(CastExpression->getSubExpr())) { if (const auto *BoolLiteral =
addFixItHintsForLiteralCastFromBool(Diagnostic, CastExpression, Context, dyn_cast<CXXBoolLiteralExpr>(Cast->getSubExpr())) {
DestinationType); Diag << tooling::fixit::createReplacement(
*Cast, getEquivalentForBoolLiteral(BoolLiteral, DestType, Context));
} else { } else {
addFixItHintsForGenericExpressionCastFromBool( fixGenericExprCastFromBool(Diag, Cast, Context, DestType.getAsString());
Diagnostic, CastExpression, Context, DestinationTypeString);
} }
} }

View File

@ -40,7 +40,7 @@ void regularImplicitCastPointerToBoolIsNotIgnored() {
int Struct::* memberPointer = &Struct::member; int Struct::* memberPointer = &Struct::member;
functionTaking<bool>(memberPointer); functionTaking<bool>(memberPointer);
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast 'int struct Struct::*' -> bool // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast 'int Struct::*' -> bool
// CHECK-FIXES: functionTaking<bool>(memberPointer != nullptr); // CHECK-FIXES: functionTaking<bool>(memberPointer != nullptr);
} }

View File

@ -20,7 +20,7 @@ void useOldNullMacroInReplacements() {
int Struct::* memberPointer = NULL; int Struct::* memberPointer = NULL;
functionTaking<bool>(!memberPointer); functionTaking<bool>(!memberPointer);
// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: implicit cast 'int struct Struct::*' -> bool // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: implicit cast 'int Struct::*' -> bool
// CHECK-FIXES: functionTaking<bool>(memberPointer == 0); // CHECK-FIXES: functionTaking<bool>(memberPointer == 0);
} }
@ -35,11 +35,11 @@ void fixFalseLiteralConvertingToNullPointer() {
// CHECK-FIXES: if (pointer == 0) {} // CHECK-FIXES: if (pointer == 0) {}
functionTaking<int Struct::*>(false); functionTaking<int Struct::*>(false);
// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit cast bool -> 'int struct Struct::*' // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit cast bool -> 'int Struct::*'
// CHECK-FIXES: functionTaking<int Struct::*>(0); // CHECK-FIXES: functionTaking<int Struct::*>(0);
int Struct::* memberPointer = NULL; int Struct::* memberPointer = NULL;
if (memberPointer != false) {} if (memberPointer != false) {}
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast bool -> 'int struct Struct::*' // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast bool -> 'int Struct::*'
// CHECK-FIXES: if (memberPointer != 0) {} // CHECK-FIXES: if (memberPointer != 0) {}
} }

View File

@ -195,7 +195,7 @@ void implicitCastToBoolSimpleCases() {
auto pointerToMember = &Struct::member; auto pointerToMember = &Struct::member;
functionTaking<bool>(pointerToMember); functionTaking<bool>(pointerToMember);
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast 'int struct Struct::*' -> bool // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit cast 'int Struct::*' -> bool
// CHECK-FIXES: functionTaking<bool>(pointerToMember != nullptr); // CHECK-FIXES: functionTaking<bool>(pointerToMember != nullptr);
} }