Use clang-tidy to add const wherever possible#16543
Conversation
There was a problem hiding this comment.
How foolproof are these checks? Were there false-positives that you had to correct manually?
Is this reliable enough we could think about adding run-clang-tidy to the chk_style job to enforce this.
There was a problem hiding this comment.
ZERO false positives in the ENTIRE codebase. ZERO.
There was a problem hiding this comment.
straight-up fixed it and recompiled and it was fine.
There was a problem hiding this comment.
Notice that the only reason why the compile is failing here is because of a warning, which is that NOW the compiler sees that something could be a for(const auto& stuff: ...) -- previously, it was copied but there was no const, so the compiler didn't know if it was changed or not and hence could not warn about this.
There was a problem hiding this comment.
ok, that sounds pretty good. Can you make a PR to hook it up into CI after we merge this?
cameel
left a comment
cameel
left a comment
There was a problem hiding this comment.
I only skimmed through the PR, but I guess there's not much that adding const can break as long as the code still compiles. The only thing I can think of is accidentally turning moves into copies. I'd only merge this if clang-tidy can guarantee not to do this.
I found some minor stuff like unused variables that should be just removed instead of being changed to const. When making corrections please keep those in a separate commit on top of what clang-tidy did - they will be impossible to notice otherwise. Same if you decide to enable any more transformations - let's keep that clearly separated from what is already here.
| std::set<CallGraph::Node, CallGraph::CompareByID> const defaultNode; | ||
| for (CallGraph::Node const& dispatchTarget: util::valueOrDefault(_creationGraph.edges, CallGraph::SpecialNode::InternalDispatch, defaultNode)) |
There was a problem hiding this comment.
This looked suspicious because we're adding const without setting any value. It's fine after all - it would normally be a temporary, and we use the variable just to extend it's lifetime - but the name makes no sense. It's not a node. Can you change it?
| std::set<CallGraph::Node, CallGraph::CompareByID> const defaultNode; | |
| for (CallGraph::Node const& dispatchTarget: util::valueOrDefault(_creationGraph.edges, CallGraph::SpecialNode::InternalDispatch, defaultNode)) | |
| std::set<CallGraph::Node, CallGraph::CompareByID> const emptyEdgeSet; | |
| for (CallGraph::Node const& dispatchTarget: util::valueOrDefault(_creationGraph.edges, CallGraph::SpecialNode::InternalDispatch, emptyEdgeSet)) |
There was a problem hiding this comment.
I don't think it's a good idea to also change names in this PR? I want to keep this PR as easy to review as possible, and hence change nothing other than const. Since this PR is large, I want to make absolutely sure it's as clean as possible, so reviewing it is as easy as possible.
| m_requestedFunctions.insert(_name); | ||
| std::string fun = _creator(); | ||
| std::string const fun = _creator(); | ||
| solAssert(!fun.empty(), ""); | ||
| solAssert(fun.find("function " + _name + "(") != std::string::npos, "Function not properly named."); | ||
| m_code += std::move(fun); |
There was a problem hiding this comment.
If you make fun const, the move turns into a copy.
Though in this case the move() is pointless as += expects a reference, not a value. It should be removed.
There was a problem hiding this comment.
I think it's probably best to not change much about the code other than adding const in this PR? Otherwise the overhead to review can be significantly greater. We can fix these post-PR I think, when the overhead to review will be much smaller -- a much smaller changeset to deal with! What do you think?
| std::string text = _args["textDocument"]["text"].get<std::string>(); | ||
| std::string uri = _args["textDocument"]["uri"].get<std::string>(); | ||
| std::string const uri = _args["textDocument"]["uri"].get<std::string>(); | ||
| m_openFiles.insert(uri); | ||
| m_fileRepository.setSourceByUri(uri, std::move(text)); |
There was a problem hiding this comment.
What happens if you if you remove std::move() here? Does clang-tidy make text const then? I'm a bit worried that if it does not account for move() we'll end up with copies in places were we do not want them. I skimmed the PR and I did not find anything like this though - there are two places where it ignored move() but move() was already ineffective there - so it would be interesting to know if that's because it's aware of move() after all.
There was a problem hiding this comment.
I think it's fine? I am not 100% sure we need to make this PR also do real semantic changes, such as removing an std::move. I think it would make it significantly harder to review, and I am not sure that's a great idea. I want to keep this a easy to review as humanly possible. The change applied here is correct, and the clang-tidy could figure out it is correct. I am terrified of changing other things, too, and then get bogged down in long-winded discussions of what else to change. Instead, I'd like to keep this PR laser-focussed, so it can go through. We can fix the other, more complicated changes, if necessary, as a follow-up PR.
|
|
||
| std::string output = std::regex_replace(_stderrContent, preReleaseWarningRegex, ""); | ||
| std::string const output = std::regex_replace(_stderrContent, preReleaseWarningRegex, ""); | ||
| return std::regex_replace(std::move(output), noOutputRegex, ""); |
There was a problem hiding this comment.
This again prevents a move, but again std:move() seems to be ineffective due to the function expecting a const reference. It should be removed.
| for (std::string contractName: compiler().contractNames()) | ||
| { | ||
| ErrorList errors; | ||
| ErrorList const errors; |
There was a problem hiding this comment.
This variable seems unused. I guess clang-tidy does not check that?
There was a problem hiding this comment.
We can enable that -- but I want to laser-focus this PR on const. I promise I will make another PR with unused variable removal, which I believe clang-tidy can also do.
| std::string BytesUtils::formatUnsigned(bytes const& _bytes) | ||
| { | ||
| std::stringstream os; | ||
| std::stringstream const os; |
There was a problem hiding this comment.
I promise I will make another PR where clang-tidy will remove unused variables. It will be a much smaller PR and hence easier to review. I want to make this PR easy to review, so we can make it work :) I pinky-promise I'll do a follow-up PR with a smaller changeset that cleans up the unused variables!
| std::stringstream serr; | ||
| size_t separatorPosition = optionName.find("="); | ||
| std::string optionNameWithoutValue = optionName.substr(0, separatorPosition); | ||
| std::stringstream const serr; |
There was a problem hiding this comment.
I promise I will make another PR where clang-tidy will remove unused variables. It will be a much smaller PR and hence easier to review. I want to make this PR easy to review, so we can make it work :) I pinky-promise I'll do a follow-up PR with a smaller changeset that cleans up the unused variables!
There was a problem hiding this comment.
If we are going to use this style, CODING_STYLE.md needs to be updated to reflect that. Especially given that this is not automatically enforced now.
There was a problem hiding this comment.
Good point! I updated the best I could. I am not very good at writing markdown. Please review!
| for (auto const& p: m_neededBy) | ||
| for (auto id: {p.first, p.second}) | ||
| if (unsigned seqNr = m_expressionClasses.representative(id).sequenceNumber) | ||
| if (unsigned const seqNr = m_expressionClasses.representative(id).sequenceNumber) |
There was a problem hiding this comment.
It does not seem to touch auto. Or function parameters. Why? Is there a separate setting for those?
There was a problem hiding this comment.
Yes, and I have ran with it and now they should be fixed. Please review :)
There was a problem hiding this comment.
it's simply not handled (yet) llvm/llvm-project#157319
cameel
changed the title
const where possible
|
By the way, cheeky titles are fine, but please make sure that they're also informative. Yours does not really say what the goal is here and the description just assumes it's obvious and runs with it. It only says "how", not "what" or "why". I mean, I can see from the diff what it's about, but it's not clear if it's all you wanted to achieve or just a starting point of something bigger. Some context would not hurt. I don't think we ever discussed this change so it came out of nowhere for me. |
msooseth
changed the title
const where possible
That's very fair. OK, I'll try to refrain from cheeky titles & give more context next time! I'll also edit this one so it explains the why? (and what for?) better! Update: made the description better. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
removed
the
stale
There was a problem hiding this comment.
I have gone through some of it and also run benchmarks in the background:
openzeppelin-5.6.1 evmasm cpu_time 20.45s ± 0.01s 20.69s ± 0.01s +1.2%
solady-0.1.26 evmasm cpu_time 29.76s ± 0.02s 30.19s ± 0.04s +1.45%
prb-math-4.1.1 evmasm cpu_time 7.17s ± 0.01s 7.28s ± 0.01s +1.63%
forge-std-1.16.1 evmasm cpu_time 9.11s ± 0.01s 9.24s ± 0.00s +1.4%
it seems this PR makes performance on evmasm consistently worse.
For IR
openzeppelin-5.6.1 ir cpu_time 56.85s ± 0.02s 57.05s ± 0.07s +0.35%
solady-0.1.26 ir cpu_time 148.86s ± 0.10s 149.31s ± 0.06s +0.31%
prb-math-4.1.1 ir cpu_time 29.37s ± 0.02s 29.36s ± 0.00s -0.04%
forge-std-1.16.1 ir cpu_time 37.25s ± 0.04s 37.33s ± 0.02s +0.21%
the performance degradation isn't quite as pronounced.
Full data
Benchmark Pipeline Metric Base Target Delta
------------------ -------- ------------- ----------------------------- ----------------------------- ------
openzeppelin-5.6.1 evmasm cpu_time 20.45s ± 0.01s 20.69s ± 0.01s +1.2%
creation_size 725,287 ± 0 725,287 ± 0 0.0%
cycles 71,095,460,466 ± 29,159,608 72,015,839,000 ± 32,713,958 +1.29%
instructions 140,218,192,678 ± 1,182,026 140,138,525,288 ± 210,001 -0.06%
peak_rss 1077 ± 0 MiB 1077 ± 0 MiB +0.03%
runtime_size 650,752 ± 0 650,752 ± 0 0.0%
wall_time 20.58s ± 0.01s 20.84s ± 0.01s +1.25%
openzeppelin-5.6.1 ir cpu_time 56.85s ± 0.02s 57.05s ± 0.07s +0.35%
creation_size 675,405 ± 0 675,405 ± 0 0.0%
cycles 201,196,458,478 ± 77,653,294 201,905,621,140 ± 258,393,158 +0.35%
instructions 321,738,263,910 ± 369,509 321,638,938,994 ± 1,633,643 -0.03%
peak_rss 1467 ± 0 MiB 1468 ± 0 MiB +0.04%
runtime_size 598,355 ± 0 598,355 ± 0 0.0%
wall_time 57.16s ± 0.02s 57.37s ± 0.08s +0.37%
solady-0.1.26 evmasm cpu_time 29.76s ± 0.02s 30.19s ± 0.04s +1.45%
creation_size 1,514,791 ± 0 1,514,791 ± 0 0.0%
cycles 103,008,393,636 ± 49,728,927 104,564,521,348 ± 109,376,161 +1.51%
instructions 214,589,544,740 ± 257,323 214,529,695,209 ± 113,727 -0.03%
peak_rss 1826 ± 0 MiB 1826 ± 0 MiB -0.05%
runtime_size 1,480,154 ± 0 1,480,154 ± 0 0.0%
wall_time 29.98s ± 0.01s 30.40s ± 0.04s +1.39%
solady-0.1.26 ir cpu_time 148.86s ± 0.10s 149.31s ± 0.06s +0.31%
creation_size 1,681,895 ± 0 1,681,895 ± 0 0.0%
cycles 528,758,621,193 ± 328,693,004 530,436,196,572 ± 201,460,879 +0.32%
instructions 784,401,436,151 ± 597,837 784,063,273,906 ± 637,251 -0.04%
peak_rss 2948 ± 0 MiB 2947 ± 0 MiB -0.03%
runtime_size 1,650,497 ± 0 1,650,497 ± 0 0.0%
wall_time 149.64s ± 0.10s 150.09s ± 0.06s +0.3%
prb-math-4.1.1 evmasm cpu_time 7.17s ± 0.01s 7.28s ± 0.01s +1.63%
creation_size 252,039 ± 0 252,039 ± 0 0.0%
cycles 23,148,303,881 ± 43,448,888 23,568,170,033 ± 28,950,615 +1.81%
instructions 49,526,996,674 ± 136,336 49,517,228,232 ± 132,989 -0.02%
peak_rss 1173 ± 0 MiB 1173 ± 0 MiB +0.02%
runtime_size 250,178 ± 0 250,178 ± 0 0.0%
wall_time 7.23s ± 0.01s 7.34s ± 0.02s +1.46%
prb-math-4.1.1 ir cpu_time 29.37s ± 0.02s 29.36s ± 0.00s -0.04%
creation_size 262,432 ± 0 262,432 ± 0 0.0%
cycles 102,297,197,050 ± 75,415,657 102,248,828,907 ± 11,268,582 -0.05%
instructions 155,252,847,130 ± 306,259 155,182,718,720 ± 137,446 -0.05%
peak_rss 1493 ± 0 MiB 1493 ± 0 MiB -0.03%
runtime_size 260,832 ± 0 260,832 ± 0 0.0%
wall_time 29.53s ± 0.02s 29.52s ± 0.00s -0.03%
forge-std-1.16.1 evmasm cpu_time 9.11s ± 0.01s 9.24s ± 0.00s +1.4%
creation_size 394,603 ± 0 394,603 ± 0 0.0%
cycles 31,488,913,241 ± 28,798,948 31,938,690,665 ± 7,458,304 +1.43%
instructions 63,224,180,849 ± 296,423 63,253,017,110 ± 167,600 +0.05%
peak_rss 533 ± 0 MiB 531 ± 0 MiB -0.36%
runtime_size 381,729 ± 0 381,729 ± 0 0.0%
wall_time 9.17s ± 0.01s 9.30s ± 0.01s +1.42%
forge-std-1.16.1 ir cpu_time 37.25s ± 0.04s 37.33s ± 0.02s +0.21%
creation_size 422,073 ± 0 422,073 ± 0 0.0%
cycles 132,013,639,898 ± 137,089,473 132,340,848,481 ± 75,689,090 +0.25%
instructions 188,394,301,511 ± 132,066 188,388,613,410 ± 104,237 -0.0%
peak_rss 828 ± 0 MiB 803 ± 0 MiB -3.04%
runtime_size 406,750 ± 0 406,750 ± 0 0.0%
wall_time 37.45s ± 0.05s 37.53s ± 0.03s +0.21%
| for (auto const& p: m_neededBy) | ||
| for (auto id: {p.first, p.second}) | ||
| if (unsigned seqNr = m_expressionClasses.representative(id).sequenceNumber) | ||
| if (unsigned const seqNr = m_expressionClasses.representative(id).sequenceNumber) |
There was a problem hiding this comment.
it's simply not handled (yet) llvm/llvm-project#157319
4 participants
What is this PR, and why did I do it?
This pr demonstrates that we can add
constto many places using the automated clang-tidy system. This uses clang and a number of rules that can be turned on/off to warn and in this case, also fix, issues. The issue I asked it to fix is places where there could be aconstadded that currently miss theconst. This revealed some issues such as places where we do unnecessary copies in range for loops, for example. More importantly, I think aconstmakes code easier to read, which is what code is mostly for.I agree that this change is large. The
.clang-tidyenables this warning in most people's LSP-s, so while writing code, it is highlighted. It may be useful to turn this on for new code, too, as a sort of quality gate, but I personally am not 100% happy about this kind of strict quality gates. However, my preferences are not the most important here.How I did this, and how can you do this yourself.
Step 1: Build with clang-tidy enabled via CMake
This integrates clang-tidy into the build — every .cpp compiled will also be checked. However, this does not apply fixes automatically.
Step 2: Apply fixes using run-clang-tidy
The better approach for auto-fixing the whole codebase is using run-clang-tidy (ships with clang-tidy) on the compile commands database:
Then you need to fix the
const auto&toauto const&via: