Skip to content

Commit 3faa5be

Browse files
5chmidti5chmidti
committed
address review comments
1 parent d3a3df2 commit 3faa5be

File tree

3 files changed

+43
-66
lines changed

3 files changed

+43
-66
lines changed

‎clang-tools-extra/clang-tidy/modernize/UseConstexprCheck.cpp‎

Lines changed: 37 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,9 @@ AST_MATCHER(FunctionDecl, locationPermitsConstexpr) {
2424
Finder->getASTContext().getSourceManager().isInMainFile(
2525
Node.getLocation());
2626

27-
if (IsInMainFile && Node.hasExternalFormalLinkage())
28-
return false;
29-
if (!IsInMainFile && !Node.isInlined())
30-
return false;
31-
32-
return true;
27+
if (IsInMainFile)
28+
return !Node.hasExternalFormalLinkage();
29+
return Node.isInlined();
3330
}
3431

3532
AST_MATCHER(Expr, isCXX11ConstantExpr) {
@@ -57,49 +54,41 @@ AST_MATCHER(Decl, hasNoRedecl) {
5754
AST_MATCHER(Decl, allRedeclsInSameFile) {
5855
const SourceManager &SM = Finder->getASTContext().getSourceManager();
5956
const SourceLocation L = Node.getLocation();
60-
for (const Decl *ReDecl : Node.redecls()) {
61-
if (!SM.isWrittenInSameFile(L, ReDecl->getLocation()))
62-
return false;
63-
}
64-
return true;
57+
return llvm::all_of(Node.redecls(), [&](const Decl *ReDecl) {
58+
return SM.isWrittenInSameFile(L, ReDecl->getLocation());
59+
});
6560
}
6661
} // namespace
6762

6863
static bool
6964
satisfiesConstructorPropertiesUntil20(const CXXConstructorDecl *Ctor,
7065
ASTContext &Ctx) {
7166
const CXXRecordDecl *Rec = Ctor->getParent();
72-
llvm::SmallPtrSet<const RecordDecl *, 8> Bases{};
73-
for (const CXXBaseSpecifier Base : Rec->bases())
74-
Bases.insert(Base.getType()->getAsRecordDecl());
7567

76-
llvm::SmallPtrSet<const FieldDecl *, 8> Fields{Rec->field_begin(),
77-
Rec->field_end()};
68+
llvm::SmallPtrSet<const FieldDecl *, 8> UninitializedFields{
69+
Rec->field_begin(), Rec->field_end()};
7870
llvm::SmallPtrSet<const FieldDecl *, 4> Indirects{};
7971

8072
for (const CXXCtorInitializer *const Init : Ctor->inits()) {
81-
const Type *InitType = Init->getBaseClass();
82-
if (InitType && InitType->isRecordType()) {
73+
if (const Type *InitType = Init->getBaseClass();
74+
InitType && InitType->isRecordType()) {
8375
const auto *ConstructingInit =
8476
llvm::dyn_cast<CXXConstructExpr>(Init->getInit());
8577
if (ConstructingInit &&
8678
!ConstructingInit->getConstructor()->isConstexprSpecified())
8779
return false;
8880
}
8981

90-
if (Init->isBaseInitializer()) {
91-
Bases.erase(Init->getBaseClass()->getAsRecordDecl());
82+
if (Init->isBaseInitializer())
9283
continue;
93-
}
9484

9585
if (Init->isMemberInitializer()) {
9686
const FieldDecl *Field = Init->getMember();
9787

9888
if (Field->isAnonymousStructOrUnion())
9989
Indirects.insert(Field);
10090

101-
Fields.erase(Field);
102-
continue;
91+
UninitializedFields.erase(Field);
10392
}
10493
}
10594

@@ -116,13 +105,10 @@ satisfiesConstructorPropertiesUntil20(const CXXConstructorDecl *Ctor,
116105
return false;
117106

118107
for (const NamedDecl *ND : IField->chain())
119-
Fields.erase(llvm::dyn_cast<FieldDecl>(ND));
108+
UninitializedFields.erase(llvm::dyn_cast<FieldDecl>(ND));
120109
}
121110

122-
if (!Fields.empty())
123-
return false;
124-
125-
return true;
111+
return UninitializedFields.empty();
126112
}
127113

128114
static bool isLiteralType(QualType QT, const ASTContext &Ctx,
@@ -161,10 +147,12 @@ static bool isLiteralType(const Type *T, const ASTContext &Ctx,
161147
}))
162148
return false;
163149

164-
for (const CXXBaseSpecifier Base : Rec->bases()) {
165-
if (!isLiteralType(Base.getType(), Ctx, ConservativeLiteralType))
166-
return false;
167-
}
150+
const auto AllBaseClassesAreLiteralTypes =
151+
llvm::all_of(Rec->bases(), [&](const CXXBaseSpecifier &Base) {
152+
return isLiteralType(Base.getType(), Ctx, ConservativeLiteralType);
153+
});
154+
if (!AllBaseClassesAreLiteralTypes)
155+
return false;
168156
}
169157

170158
if (const Type *ArrayElementType = T->getArrayElementTypeNoTypeQual())
@@ -193,17 +181,9 @@ static bool satisfiesProperties11(
193181
!AddConstexprToMethodOfClassWithoutConstexprConstructor)
194182
return false;
195183

196-
if (Method &&
197-
(Method->isVirtual() ||
198-
!match(cxxMethodDecl(hasBody(cxxTryStmt())), *Method, Ctx).empty()))
199-
return false;
200-
201184
if (const auto *Ctor = llvm::dyn_cast<CXXConstructorDecl>(FDecl);
202185
Ctor && (!satisfiesConstructorPropertiesUntil20(Ctor, Ctx) ||
203-
llvm::any_of(Ctor->getParent()->bases(),
204-
[](const CXXBaseSpecifier &Base) {
205-
return Base.isVirtual();
206-
})))
186+
Ctor->getParent()->getNumVBases() > 0))
207187
return false;
208188

209189
if (const auto *Dtor = llvm::dyn_cast<CXXDestructorDecl>(FDecl);
@@ -213,9 +193,12 @@ static bool satisfiesProperties11(
213193
if (!isLiteralType(FDecl->getReturnType(), Ctx, ConservativeLiteralType))
214194
return false;
215195

216-
for (const ParmVarDecl *Param : FDecl->parameters())
217-
if (!isLiteralType(Param->getType(), Ctx, ConservativeLiteralType))
218-
return false;
196+
const bool OnlyHasParametersOfLiteralType =
197+
llvm::all_of(FDecl->parameters(), [&](const ParmVarDecl *Param) {
198+
return isLiteralType(Param->getType(), Ctx, ConservativeLiteralType);
199+
});
200+
if (!OnlyHasParametersOfLiteralType)
201+
return false;
219202

220203
class Visitor11 : public clang::RecursiveASTVisitor<Visitor11> {
221204
public:
@@ -225,10 +208,7 @@ static bool satisfiesProperties11(
225208
Visitor11(ASTContext &Ctx, bool ConservativeLiteralType)
226209
: Ctx(Ctx), ConservativeLiteralType(ConservativeLiteralType) {}
227210

228-
bool WalkUpFromNullStmt(NullStmt *) {
229-
Possible = false;
230-
return false;
231-
}
211+
bool WalkUpFromNullStmt(NullStmt *) { return false; }
232212
bool WalkUpFromDeclStmt(DeclStmt *DS) {
233213
for (const Decl *D : DS->decls())
234214
if (!llvm::isa<StaticAssertDecl, TypedefNameDecl, UsingDecl,
@@ -336,10 +316,7 @@ static bool satisfiesProperties11(
336316

337317
Visitor11 V{Ctx, ConservativeLiteralType};
338318
V.TraverseDecl(const_cast<FunctionDecl *>(FDecl));
339-
if (!V.Possible)
340-
return false;
341-
342-
return true;
319+
return V.Possible;
343320
}
344321

345322
namespace {
@@ -368,12 +345,10 @@ AST_MATCHER_P(VarDecl, satisfiesVariableProperties, bool,
368345
if (!isLiteralType(QT, Ctx, ConservativeLiteralType))
369346
return false;
370347

371-
const bool IsDeclaredInsideConstexprFunction = std::invoke([&Node]() {
348+
const bool IsDeclaredInsideConstexprFunction = [&Node]() {
372349
const auto *Func = llvm::dyn_cast<FunctionDecl>(Node.getDeclContext());
373-
if (!Func)
374-
return false;
375-
return Func->isConstexpr();
376-
});
350+
return Func && Func->isConstexpr();
351+
}();
377352

378353
if (Node.isStaticLocal() && IsDeclaredInsideConstexprFunction)
379354
return false;
@@ -386,10 +361,8 @@ AST_MATCHER_P(VarDecl, satisfiesVariableProperties, bool,
386361
if (ArrayOrPtrElement)
387362
RDecl = ArrayOrPtrElement->getAsCXXRecordDecl();
388363

389-
if (RDecl && (!RDecl->hasDefinition() || !RDecl->hasConstexprDestructor()))
390-
return false;
391-
392-
return true;
364+
return !(RDecl &&
365+
(!RDecl->hasDefinition() || !RDecl->hasConstexprDestructor()));
393366
}
394367
} // namespace
395368

@@ -398,7 +371,7 @@ void UseConstexprCheck::registerMatchers(MatchFinder *Finder) {
398371
functionDecl(
399372
isDefinition(),
400373
unless(anyOf(isConstexpr(), isImplicit(), hasExternalFormalLinkage(),
401-
isInMacro(), isMain(), isInStdNamespace(),
374+
isInMacro(), isMain(), isDeleted(), isInStdNamespace(),
402375
isExpansionInSystemHeader(), isExternC(),
403376
cxxMethodDecl(ofClass(cxxRecordDecl(isLambda()))))),
404377
locationPermitsConstexpr(), allRedeclsInSameFile(),
@@ -467,8 +440,8 @@ void UseConstexprCheck::onEndOfTranslationUnit() {
467440
FunctionReplacement);
468441
}
469442

470-
const auto MaybeRemoveConst = [&, this](DiagnosticBuilder &Diag,
471-
const VarDecl *Var) {
443+
const auto MaybeRemoveConst = [&](DiagnosticBuilder &Diag,
444+
const VarDecl *Var) {
472445
// Since either of the locs can be in a macro, use `makeFileCharRange` to be
473446
// sure that we have a consistent `CharSourceRange`, located entirely in the
474447
// source file.

‎clang-tools-extra/docs/clang-tidy/checks/modernize/use-constexpr.rst‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ Options
3838
.. option:: ConservativeLiteralType
3939

4040
With this option enabled, only literal types that can be constructed at
41-
compile-time are considered to supoprt ``constexpr``.
41+
compile-time are considered to support ``constexpr``.
4242

4343
.. code-block:: c++
4444

@@ -49,7 +49,7 @@ Options
4949
};
5050

5151
This type is a literal type, but can not be constructed at compile-time.
52-
With `ConservativeLiteralType` equal to `true`, variables or funtions
52+
With :option:`ConservativeLiteralType` equal to `true`, variables or functions
5353
with this type are not diagnosed to add ``constexpr``. Default is
5454
`true`.
5555

‎clang-tools-extra/test/clang-tidy/checkers/modernize/use-constexpr.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ namespace function {
5959
// CHECK-FIXES-11: static constexpr int f5(Empty x) { return 0; }
6060

6161
static int f6(Empty x) { ; return 0; }
62+
// CHECK-MESSAGES-11: :[[@LINE-1]]:14: warning: declare function 'f6' as 'constexpr'
63+
// CHECK-FIXES-11: static constexpr int f6(Empty x) { ; return 0; }
6264

6365
static int f7(Empty x) { static_assert(0 == 0, ""); return 0; }
6466
// CHECK-MESSAGES-11: :[[@LINE-1]]:14: warning: declare function 'f7' as 'constexpr'
@@ -231,6 +233,8 @@ namespace function_non_literal_ref {
231233
// CHECK-FIXES-11-CLT: static constexpr int f5(NonLiteral& x) { return 0; }
232234

233235
static int f6(NonLiteral& x) { ; return 0; }
236+
// CHECK-MESSAGES-11-CLT: :[[@LINE-1]]:14: warning: declare function 'f6' as 'constexpr'
237+
// CHECK-FIXES-11-CLT: static constexpr int f6(NonLiteral& x) { ; return 0; }
234238

235239
static int f7(NonLiteral& x) { static_assert(0 == 0, ""); return 0; }
236240
// CHECK-MESSAGES-11-CLT: :[[@LINE-1]]:14: warning: declare function 'f7' as 'constexpr'

0 commit comments

Comments
 (0)