[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing and giving suggestions! @Febbe

Please take a look at my reply. Sorry if I misunderstood anything!



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:215
                   (Twine("static_cast<") + TyAsString + ">(").str())
-           << FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
     else
----------------
Febbe wrote:
> Actually, I find it pretty weird/suspicious, that `<AnyBinaryExr*> -> getEndLoc()` does not return the end of its `Expr`. The code looked correct already.
> Can you elaborate, if this is a bug in the `getEndLoc()` of those `Expr`s? It might be better to fix it there directly.
> 
I'm really not an expert on `SourceLocation`. I just took a rough look at these `getEndLoc()`s.

Take `1 + 2` as an example, this is a `BinaryOperator` with two `IntegerLiteral`s. When we call `Expr::getEndLoc()` which is actually `Stmt::getEndLoc()` on it, it will call `BinaryOperator::getEndLoc()` and this actually returns the `RHS->getEndLoc()`, that is  the result of `IntegerLiteral(2)->getEndLoc()`. And the interesting stuff is that `IntegerLiteral::getBeginLoc()` and `IntegerLiteral::getEndLoc()` return the same `Loc`. Although I'm not sure which location these will return, I guess that's why `BinaryOperator::getEndLoc()` doesn't return the end of the expression.

Sorry if I misunderstood anything! After all, I didn't find documents saying which location these `Expr`s' `getEndLoc()` should return. I think it's better to have experts take a look at this.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp:18
   // CHECK-NOTES-C:                    (ptrdiff_t)( )
   // CHECK-NOTES-CXX:                  static_cast<ptrdiff_t>( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
----------------
Actually I have tried adding

```
// CHECK-FIXES-C: return &base[(ptrdiff_t)(a * b)];
// CHECK-FIXES-CXX: return &base[static_cast<ptrdiff_t>(a * b)];
```

under this line, but the test failed, and when I took a look at `build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/implicit-widening-of-multiplication-result-array-subscript-expression.cpp.tmp.cpp`, I found that these codes didn't get modified. And I took a look at other files which have `CHECK-FIXES` lines, I found the codes in the corresponding temporary files got fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141058/new/

https://reviews.llvm.org/D141058