Differential D141058
[clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result Authored by v1nh1shungry on Jan 5 2023, 7:24 AM.
Details Relevant issue: https://github.com/llvm/llvm-project/issues/56728 Currently, this check will give some wrong fix-hints, e.g. long long foobar() {
return 1024 * 1024;
// one of the fix-hint will lead to `return static_cast<long long>(1024 * )1024;
// another one will lead to `return static_cast<long long>()1204 * 1024;
}
Diff Detail
Event TimelineComment Actions Sorry, I don't know how to add tests for such fixes. Could anyone please give me some hints? Thanks! Comment Actions It seems like the tests for this check do not contain checks for the fixups itself. Normally, they are tested with CHECK-FIXES: <Fixed line above, whitespaces truncated at the beginning and the end> for example: return F? Mov: Movable{};
// CHECK-MESSAGES: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
// CHECK-FIXES: return F? std::move(Mov): Movable{};In the case of this test file, you have to append the suffixes to the CHECK-FIXES to CHECK-FIXES-<SUFFIX> where <SUFFIX> is one of C, CXX, ALL return F? Mov: Movable{};
// CHECK-MESSAGES-CXX: [[@LINE-1]]:13: warning: Parameter 'Mov' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
// CHECK-FIXES-CXX: return F? std::move(Mov): Movable{};
Comment Actions Thank you for reviewing and giving suggestions! @Febbe Please take a look at my reply. Sorry if I misunderstood anything!
Comment Actions I just made a test: #include <cstdint>
#include <vector>
int64_t f(int x) {
return 1024 * 1024 * 1024 * x;
}
void g(int x) {
std::vector<int> b;
b.reserve(1024 * 1024 * 1024 * x);
}
int main() {
f(1024);
g(1024);
}The fixed code: #include <cstddef>
#include <cstdint>
#include <vector>
int64_t f(int x) {
return static_cast<int64_t>(1024 * 1024 * 1024 * x);
}
void g(int x) {
std::vector<int> b;
b.reserve(static_cast<size_type>(1024 * 1024 * 1024 * x));
}
int main() {
f(1024);
g(1024);
}For the first test, I still think that the auto fix should be return static_cast<int64_t>(1024) * 1024 * 1024 * x; to have the good result. For the second test, the type is the internal type of std::vector. Comment Actions Thank you for taking a look at this patch! @bansan
I'd say I'm not the author of this checker. I think this behavior change needs a discussion somewhere, for example, you could raise an issue on GitHub. I just want to fix the wrong fix-hint and I think this is out of what this patch should do. Sorry!
Good catch, thanks! I didn't realize this problem. Will take a look at it. Comment Actions
There are two approaches to fixing this issue. One is fixing with the FULLY qualified type name, in the above case that is, std::vector<int, std::allocator<int>>::size_type. Another one is fixing with the desugared type name, that is, unsigned long. I personally don't have a strong opinion on which one is better. WDYT, @bansan? Comment Actions
IMHO, the first approach can cause redundancy in some cases, like this one. The second one will turn int64_t into long, which I don't have a good feeling about. Comment Actions The current rule is quite simple. I tried this example: template<typename T>
T h(int x) {
return 1024 * 1024 * 1024 * x;
}
int main() {
h<int64_t>(1024);
}The rule does not complain. So I think that the "resolved" type (size_t) should be use instead of the templated one (std::vector<int, std::allocator<int>>::size_type). Comment Actions
Sorry, I'm confused. Could you please explain what you expect this example should achieve and what is "the rule"? Thanks!
Note: the "desugared type" I mention is the FULLY desugared type, so it will achieve unsigned long instead of size_t. There is a method QualType::getSingleStepDesugaredType() which may get size_t, but I haven't had a try. But even though it works, this will still turn int64_t into long. BTW, if you think the desugared type is better, which do you think is better, to get single step desugared or to get fully desugared? Comment Actions
Ah, I think I understand it. You mean the bugprone-implicit-widening-of-multiplication-result check doesn't warn anything about this example, right? If so, although I don't understand what you mean by coming up with this example, to classify, when I said
the point is that if we choose to use the desugared type, int64_t foobar() {
return 1024 * 1024;
}will change to int64_t foobar() {
return static_cast<long>(1024 * 1024);
^^^^ instead of int64_t
}Here I think this will break the portability. Comment Actions
Just did a quick test, and it doesn't work like this. So I can only get the fully desugared type for now. Sorry :( Comment Actions About my template example: I wanted to say that the actual bugprone-implicit-widening-of-multiplication-result rule looks to not analyze template calculation problem. So I think it's better to use the desugared type (size_t). It's not acceptable (IMHO) that the hard-coded size_t is resolved as long. According to https://en.cppreference.com/w/cpp/language/types long may be 32 bits. Unfortunately, I don't know anything about AST and which function you should use :/ Comment Actions
Hmm, I don't think this two are related.
Yeah, I don't have a good feeling about it too. The point is that we love desugaring size_type to size_t (which I haven't found a way to achieve yet), and hate desugaring int64_t to long, but how can we classify them? They BOTH are the desugared type. According to the conversation above, I'd use the qualified alias type name.
| ||||||||||||||||||||||||
Please don't use auto unless type is explicitly stated in same statement or iterator.