Skip to content

Commit 0e55fef

Browse files
njames93PiotrZSL
njames93
authored and
PiotrZSL
committed
[clang-tidy] Tweak diag ranges for bugprone-sizeof-expression
Provide more useful warning locations and diagnostic ranges. Reviewed By: PiotrZSL Differential Revision: https://reviews.llvm.org/D101617
1 parent 9311d12 commit 0e55fef

File tree

5 files changed

+115
-87
lines changed

5 files changed

+115
-87
lines changed

‎clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp

Lines changed: 77 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -192,10 +192,12 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
192192
}
193193

194194
// Detect expression like: sizeof(expr, expr); most likely an error.
195-
Finder->addMatcher(sizeOfExpr(has(ignoringParenImpCasts(
196-
binaryOperator(hasOperatorName(",")))))
197-
.bind("sizeof-comma-expr"),
198-
this);
195+
Finder->addMatcher(
196+
sizeOfExpr(
197+
has(ignoringParenImpCasts(
198+
binaryOperator(hasOperatorName(",")).bind("sizeof-comma-binop"))))
199+
.bind("sizeof-comma-expr"),
200+
this);
199201

200202
// Detect sizeof(...) /sizeof(...));
201203
// FIXME:
@@ -255,51 +257,62 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
255257
Finder->addMatcher(
256258
binaryOperator(
257259
hasAnyOperatorName("==", "!=", "<", "<=", ">", ">=", "+", "-"),
258-
hasOperands(
259-
anyOf(ignoringParenImpCasts(SizeOfExpr),
260-
ignoringParenImpCasts(binaryOperator(
261-
hasOperatorName("*"),
262-
hasEitherOperand(ignoringParenImpCasts(SizeOfExpr))))),
263-
ignoringParenImpCasts(PtrDiffExpr)))
260+
hasOperands(anyOf(ignoringParenImpCasts(
261+
SizeOfExpr.bind("sizeof-ptr-mul-expr")),
262+
ignoringParenImpCasts(binaryOperator(
263+
hasOperatorName("*"),
264+
hasEitherOperand(ignoringParenImpCasts(
265+
SizeOfExpr.bind("sizeof-ptr-mul-expr")))))),
266+
ignoringParenImpCasts(PtrDiffExpr)))
264267
.bind("sizeof-in-ptr-arithmetic-mul"),
265268
this);
266269

267-
Finder->addMatcher(binaryOperator(hasOperatorName("/"),
268-
hasLHS(ignoringParenImpCasts(PtrDiffExpr)),
269-
hasRHS(ignoringParenImpCasts(SizeOfExpr)))
270-
.bind("sizeof-in-ptr-arithmetic-div"),
271-
this);
270+
Finder->addMatcher(
271+
binaryOperator(
272+
hasOperatorName("/"), hasLHS(ignoringParenImpCasts(PtrDiffExpr)),
273+
hasRHS(ignoringParenImpCasts(SizeOfExpr.bind("sizeof-ptr-div-expr"))))
274+
.bind("sizeof-in-ptr-arithmetic-div"),
275+
this);
272276
}
273277

274278
void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
275279
const ASTContext &Ctx = *Result.Context;
276280

277281
if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-constant")) {
278-
diag(E->getBeginLoc(),
279-
"suspicious usage of 'sizeof(K)'; did you mean 'K'?");
282+
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(K)'; did you mean 'K'?")
283+
<< E->getSourceRange();
280284
} else if (const auto *E =
281285
Result.Nodes.getNodeAs<Expr>("sizeof-integer-call")) {
282286
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
283-
"that results in an integer");
287+
"that results in an integer")
288+
<< E->getSourceRange();
284289
} else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-this")) {
285290
diag(E->getBeginLoc(),
286-
"suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'");
291+
"suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'")
292+
<< E->getSourceRange();
287293
} else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-charp")) {
288294
diag(E->getBeginLoc(),
289-
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?");
295+
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
296+
<< E->getSourceRange();
290297
} else if (const auto *E =
291298
Result.Nodes.getNodeAs<Expr>("sizeof-pointer-to-aggregate")) {
292299
diag(E->getBeginLoc(),
293-
"suspicious usage of 'sizeof(A*)'; pointer to aggregate");
294-
} else if (const auto *E =
295-
Result.Nodes.getNodeAs<Expr>("sizeof-compare-constant")) {
296-
diag(E->getBeginLoc(),
297-
"suspicious comparison of 'sizeof(expr)' to a constant");
300+
"suspicious usage of 'sizeof(A*)'; pointer to aggregate")
301+
<< E->getSourceRange();
302+
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
303+
"sizeof-compare-constant")) {
304+
diag(E->getOperatorLoc(),
305+
"suspicious comparison of 'sizeof(expr)' to a constant")
306+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
298307
} else if (const auto *E =
299308
Result.Nodes.getNodeAs<Expr>("sizeof-comma-expr")) {
300-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(..., ...)'");
309+
const auto *BO =
310+
Result.Nodes.getNodeAs<BinaryOperator>("sizeof-comma-binop");
311+
assert(BO);
312+
diag(BO->getOperatorLoc(), "suspicious usage of 'sizeof(..., ...)'")
313+
<< E->getSourceRange();
301314
} else if (const auto *E =
302-
Result.Nodes.getNodeAs<Expr>("sizeof-divide-expr")) {
315+
Result.Nodes.getNodeAs<BinaryOperator>("sizeof-divide-expr")) {
303316
const auto *NumTy = Result.Nodes.getNodeAs<Type>("num-type");
304317
const auto *DenomTy = Result.Nodes.getNodeAs<Type>("denom-type");
305318
const auto *ElementTy = Result.Nodes.getNodeAs<Type>("elem-type");
@@ -311,49 +324,64 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
311324

312325
if (DenominatorSize > CharUnits::Zero() &&
313326
!NumeratorSize.isMultipleOf(DenominatorSize)) {
314-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
315-
" numerator is not a multiple of denominator");
327+
diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
328+
" numerator is not a multiple of denominator")
329+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
316330
} else if (ElementSize > CharUnits::Zero() &&
317331
DenominatorSize > CharUnits::Zero() &&
318332
ElementSize != DenominatorSize) {
319-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
320-
" numerator is not a multiple of denominator");
333+
diag(E->getOperatorLoc(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
334+
" numerator is not a multiple of denominator")
335+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
321336
} else if (NumTy && DenomTy && NumTy == DenomTy) {
322-
diag(E->getBeginLoc(),
323-
"suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'");
337+
diag(E->getOperatorLoc(),
338+
"suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
339+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
324340
} else if (PointedTy && DenomTy && PointedTy == DenomTy) {
325-
diag(E->getBeginLoc(),
326-
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'");
341+
diag(E->getOperatorLoc(),
342+
"suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
343+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
327344
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
328345
DenomTy->isPointerType()) {
329-
diag(E->getBeginLoc(),
330-
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'");
346+
diag(E->getOperatorLoc(),
347+
"suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
348+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
331349
}
332350
} else if (const auto *E =
333351
Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {
334-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(sizeof(...))'");
335-
} else if (const auto *E =
336-
Result.Nodes.getNodeAs<Expr>("sizeof-multiply-sizeof")) {
337-
diag(E->getBeginLoc(), "suspicious 'sizeof' by 'sizeof' multiplication");
338-
} else if (const auto *E =
339-
Result.Nodes.getNodeAs<Expr>("sizeof-in-ptr-arithmetic-mul")) {
352+
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(sizeof(...))'")
353+
<< E->getSourceRange();
354+
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
355+
"sizeof-multiply-sizeof")) {
356+
diag(E->getOperatorLoc(), "suspicious 'sizeof' by 'sizeof' multiplication")
357+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
358+
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
359+
"sizeof-in-ptr-arithmetic-mul")) {
340360
const auto *LPtrTy = Result.Nodes.getNodeAs<Type>("left-ptr-type");
341361
const auto *RPtrTy = Result.Nodes.getNodeAs<Type>("right-ptr-type");
342362
const auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
363+
const auto *SizeOfExpr =
364+
Result.Nodes.getNodeAs<UnaryExprOrTypeTraitExpr>("sizeof-ptr-mul-expr");
343365

344366
if ((LPtrTy == RPtrTy) && (LPtrTy == SizeofArgTy)) {
345-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
346-
"pointer arithmetic");
367+
diag(SizeOfExpr->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
368+
"pointer arithmetic")
369+
<< SizeOfExpr->getSourceRange() << E->getOperatorLoc()
370+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
347371
}
348-
} else if (const auto *E =
349-
Result.Nodes.getNodeAs<Expr>("sizeof-in-ptr-arithmetic-div")) {
372+
} else if (const auto *E = Result.Nodes.getNodeAs<BinaryOperator>(
373+
"sizeof-in-ptr-arithmetic-div")) {
350374
const auto *LPtrTy = Result.Nodes.getNodeAs<Type>("left-ptr-type");
351375
const auto *RPtrTy = Result.Nodes.getNodeAs<Type>("right-ptr-type");
352376
const auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
377+
const auto *SizeOfExpr =
378+
Result.Nodes.getNodeAs<UnaryExprOrTypeTraitExpr>("sizeof-ptr-div-expr");
353379

354380
if ((LPtrTy == RPtrTy) && (LPtrTy == SizeofArgTy)) {
355-
diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
356-
"pointer arithmetic");
381+
diag(SizeOfExpr->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
382+
"pointer arithmetic")
383+
<< SizeOfExpr->getSourceRange() << E->getOperatorLoc()
384+
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
357385
}
358386
}
359387
}

‎clang-tools-extra/clangd/test/diagnostics-tidy.test

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'?",
1515
# CHECK-NEXT: "range": {
1616
# CHECK-NEXT: "end": {
17-
# CHECK-NEXT: "character": 12,
17+
# CHECK-NEXT: "character": 16,
1818
# CHECK-NEXT: "line": 1
1919
# CHECK-NEXT: },
2020
# CHECK-NEXT: "start": {

‎clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ TEST(DiagnosticsTest, ClangTidy) {
305305
int $main[[main]]() {
306306
int y = 4;
307307
return SQUARE($macroarg[[++]]y);
308-
return $doubled[[sizeof]](sizeof(int));
308+
return $doubled[[sizeof(sizeof(int))]];
309309
}
310310
311311
// misc-no-recursion uses a custom traversal from the TUDecl

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,9 @@ int Test5() {
6868
sum += sizeof(A10) / sizeof(PtrArray[0]);
6969
// No warning.
7070
sum += sizeof(PC) / sizeof(PtrArray[0]);
71-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
71+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
7272
sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
73-
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
73+
// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
7474

7575
return sum;
7676
}

0 commit comments

Comments
 (0)