Skip to content

Commit

Permalink
[clang-tidy] Simplify unused RAII check
Browse files Browse the repository at this point in the history 
Fix handling of default construction where the constructor has a default arg.

Differential Revision: https://reviews.llvm.org/D97142
  • Loading branch information
@steveire
steveire committed Mar 2, 2021
1 parent 3d8f842 commit 7b6fc9a
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 38 deletions.
83 changes: 46 additions & 37 deletions clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp
View file
Expand Up @@ -25,25 +25,37 @@ AST_MATCHER(CXXRecordDecl, hasNonTrivialDestructor) {

void UnusedRaiiCheck::registerMatchers(MatchFinder *Finder) {
// Look for temporaries that are constructed in-place and immediately
// destroyed. Look for temporaries created by a functional cast but not for
// those returned from a call.
auto BindTemp = cxxBindTemporaryExpr(
unless(has(ignoringParenImpCasts(callExpr()))),
unless(has(ignoringParenImpCasts(objcMessageExpr()))))
.bind("temp");
// destroyed.
Finder->addMatcher(
traverse(TK_AsIs,
exprWithCleanups(
unless(isInTemplateInstantiation()),
hasParent(compoundStmt().bind("compound")),
hasType(cxxRecordDecl(hasNonTrivialDestructor())),
anyOf(has(ignoringParenImpCasts(BindTemp)),
has(ignoringParenImpCasts(cxxFunctionalCastExpr(
has(ignoringParenImpCasts(BindTemp)))))))
.bind("expr")),
mapAnyOf(cxxConstructExpr, cxxUnresolvedConstructExpr)
.with(hasParent(compoundStmt().bind("compound")),
anyOf(hasType(cxxRecordDecl(hasNonTrivialDestructor())),
hasType(templateSpecializationType(
hasDeclaration(classTemplateDecl(has(
cxxRecordDecl(hasNonTrivialDestructor()))))))))
.bind("expr"),
this);
}

template <typename T>
void reportDiagnostic(DiagnosticBuilder D, const T *Node, SourceRange SR,
bool DefaultConstruction) {
const char *Replacement = " give_me_a_name";

// If this is a default ctor we have to remove the parens or we'll introduce a
// most vexing parse.
if (DefaultConstruction) {
D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(SR),
Replacement);
return;
}

// Otherwise just suggest adding a name. To find the place to insert the name
// find the first TypeLoc in the children of E, which always points to the
// written type.
D << FixItHint::CreateInsertion(SR.getBegin(), Replacement);
}

void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) {
const auto *E = Result.Nodes.getNodeAs<Expr>("expr");

Expand All @@ -55,35 +67,32 @@ void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) {
// Don't emit a warning for the last statement in the surrounding compound
// statement.
const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("compound");
if (E == CS->body_back())
const auto *LastExpr = dyn_cast<Expr>(CS->body_back());

if (LastExpr && E == LastExpr->IgnoreUnlessSpelledInSource())
return;

// Emit a warning.
auto D = diag(E->getBeginLoc(), "object destroyed immediately after "
"creation; did you mean to name the object?");
const char *Replacement = " give_me_a_name";

// If this is a default ctor we have to remove the parens or we'll introduce a
// most vexing parse.
const auto *BTE = Result.Nodes.getNodeAs<CXXBindTemporaryExpr>("temp");
if (const auto *TOE = dyn_cast<CXXTemporaryObjectExpr>(BTE->getSubExpr()))
if (TOE->getNumArgs() == 0) {
D << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(TOE->getParenOrBraceRange()),
Replacement);
return;
if (const auto *Node = dyn_cast<CXXConstructExpr>(E))
reportDiagnostic(D, Node, Node->getParenOrBraceRange(),
Node->getNumArgs() == 0 ||
isa<CXXDefaultArgExpr>(Node->getArg(0)));
if (const auto *Node = dyn_cast<CXXUnresolvedConstructExpr>(E)) {
auto SR = SourceRange(Node->getLParenLoc(), Node->getRParenLoc());
auto DefaultConstruction = Node->getNumArgs() == 0;
if (!DefaultConstruction) {
auto FirstArg = Node->getArg(0);
DefaultConstruction = isa<CXXDefaultArgExpr>(FirstArg);
if (auto ILE = dyn_cast<InitListExpr>(FirstArg)) {
DefaultConstruction = ILE->getNumInits() == 0;
SR = SourceRange(ILE->getLBraceLoc(), ILE->getRBraceLoc());
}
}

// Otherwise just suggest adding a name. To find the place to insert the name
// find the first TypeLoc in the children of E, which always points to the
// written type.
auto Matches =
match(expr(hasDescendant(typeLoc().bind("t"))), *E, *Result.Context);
if (const auto *TL = selectFirst<TypeLoc>("t", Matches))
D << FixItHint::CreateInsertion(
Lexer::getLocForEndOfToken(TL->getEndLoc(), 0, *Result.SourceManager,
getLangOpts()),
Replacement);
reportDiagnostic(D, Node, SR, DefaultConstruction);
}
}

} // namespace bugprone
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h
View file
Expand Up @@ -28,6 +28,9 @@ class UnusedRaiiCheck : public ClangTidyCheck {
}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}
};

} // namespace bugprone
Expand Down
54 changes: 53 additions & 1 deletion clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp
View file
@@ -1,4 +1,4 @@
// RUN: %check_clang_tidy %s bugprone-unused-raii %t
// RUN: %check_clang_tidy %s bugprone-unused-raii %t -- -- -fno-delayed-template-parsing

struct Foo {
Foo();
Expand Down Expand Up @@ -46,6 +46,42 @@ void neverInstantiated() {
T();
}

struct CtorDefaultArg {
CtorDefaultArg(int i = 0);
~CtorDefaultArg();
};

template <typename T>
struct TCtorDefaultArg {
TCtorDefaultArg(T i = 0);
~TCtorDefaultArg();
};

struct CtorTwoDefaultArg {
CtorTwoDefaultArg(int i = 0, bool b = false);
~CtorTwoDefaultArg();
};

template <typename T>
void templatetest() {
TCtorDefaultArg<T>();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name;
TCtorDefaultArg<T>{};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name;

TCtorDefaultArg<T>(T{});
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name(T{});
TCtorDefaultArg<T>{T{}};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name{T{}};

int i = 0;
(void)i;
}

void test() {
Foo(42);
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
Expand All @@ -71,6 +107,22 @@ void test() {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: FooBar give_me_a_name;

CtorDefaultArg();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: CtorDefaultArg give_me_a_name;

CtorTwoDefaultArg();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: CtorTwoDefaultArg give_me_a_name;

TCtorDefaultArg<int>();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<int> give_me_a_name;

TCtorDefaultArg<int>{};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
// CHECK-FIXES: TCtorDefaultArg<int> give_me_a_name;

templ<FooBar>();
templ<Bar>();

Expand Down

0 comments on commit 7b6fc9a

Please sign in to comment.