Use clang-tidy to add const wherever possible by msooseth · Pull Request #16543 · argotorg/solidity

msooseth · 2026-03-24T13:29:21Z

What is this PR, and why did I do it?

This pr demonstrates that we can add const to 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 a const added that currently miss the const. This revealed some issues such as places where we do unnecessary copies in range for loops, for example. More importantly, I think a const makes code easier to read, which is what code is mostly for.

I agree that this change is large. The .clang-tidy enables 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

  cd solidity
  mkdir build-clang-tidy && cd build-clang-tidy
  cmake .. -DCMAKE_BUILD_TYPE=Debug \
           -DCMAKE_CXX_CLANG_TIDY="clang-tidy" \
           -DCMAKE_EXPORT_COMPILE_COMMANDS=ON

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:

  # Generate compile_commands.json (if not done above)
  cd solidity/build-clang-tidy
  cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
  CCACHE_DISABLE=1 make -j8   # need a successful build first for generated headers

  # Run clang-tidy with auto-fix on all project sources
  cd solidity
  run-clang-tidy -p build-clang-tidy \
                 -checks='-*,misc-const-correctness,performance-for-range-copy,readability-make-member-function-const' \
                 -header-filter='.*' \
                 -fix \
                 -j8 \
                 'lib.*|solc/'

Then you need to fix the const auto& to auto const& via:

find solidity/ -name '*.h' -exec sed -E -i 's/const auto&/auto const\&/g' {} +
find solidity/ -name '*.cpp' -exec sed -E -i 's/const auto&/auto const\&/g' {} +

cameel · 2026-03-24T16:02:34Z

.clang-tidy

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.

ZERO false positives in the ENTIRE codebase. ZERO.

straight-up fixed it and recompiled and it was fine.

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.

ok, that sounds pretty good. Can you make a PR to hook it up into CI after we merge this?

cameel

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.

cameel · 2026-03-25T11:07:00Z

libsolidity/analysis/FunctionCallGraph.cpp

+	std::set<CallGraph::Node, CallGraph::CompareByID> const defaultNode;
 	for (CallGraph::Node const& dispatchTarget: util::valueOrDefault(_creationGraph.edges, CallGraph::SpecialNode::InternalDispatch, defaultNode))


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?

Suggested change

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))

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.

cameel · 2026-03-25T11:46:01Z

libsolidity/codegen/MultiUseYulFunctionCollector.cpp

 		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);


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.

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?

cameel · 2026-03-25T11:52:01Z

libsolidity/lsp/LanguageServer.cpp

 		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));


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.

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.

cameel · 2026-03-25T11:56:26Z

test/libsolidity/util/Common.cpp


-	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, "");


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.

cameel · 2026-03-25T12:38:52Z

test/libsolidity/MemoryGuardTest.cpp

 	for (std::string contractName: compiler().contractNames())
 	{
-		ErrorList errors;
+		ErrorList const errors;


This variable seems unused. I guess clang-tidy does not check that?

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.

cameel · 2026-03-25T12:44:58Z

test/libsolidity/util/BytesUtils.cpp

 std::string BytesUtils::formatUnsigned(bytes const& _bytes)
 {
-	std::stringstream os;
+	std::stringstream const os;


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!

cameel · 2026-03-25T12:56:52Z

test/solc/CommandLineParser.cpp

-			std::stringstream serr;
-			size_t separatorPosition = optionName.find("=");
-			std::string optionNameWithoutValue = optionName.substr(0, separatorPosition);
+			std::stringstream const serr;


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!

cameel · 2026-03-25T13:02:54Z

.clang-tidy

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.

Good point! I updated the best I could. I am not very good at writing markdown. Please review!

cameel · 2026-03-25T13:04:17Z

libevmasm/CommonSubexpressionEliminator.cpp

 	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)


It does not seem to touch auto. Or function parameters. Why? Is there a separate setting for those?

Yes, and I have ran with it and now they should be fixed. Please review :)

cameel · 2026-03-25T13:20:46Z

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 · 2026-03-25T13:47:49Z

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.

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.

libevmasm/ConstantOptimiser.cpp

libevmasm/ControlFlowGraph.cpp

libsolidity/analysis/ControlFlowBuilder.cpp

libsolidity/analysis/DeclarationContainer.cpp

libsolidity/analysis/NameAndTypeResolver.cpp

libsolidity/analysis/SyntaxChecker.cpp

libsolidity/codegen/ContractCompiler.cpp

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

libsolidity/experimental/analysis/Analysis.cpp

libsolidity/experimental/analysis/TypeInference.cpp

libsolidity/experimental/ast/Type.cpp

libsolidity/experimental/ast/TypeSystemHelper.cpp

Add clang-tidy to warn for future

cameel reviewed Mar 24, 2026

View reviewed changes

cameel reviewed Mar 25, 2026

View reviewed changes

cameel changed the title ~~You get a const, you get a const, EVERYBODY GETS A CONST~~ You get a const, you get a const, EVERYBODY GETS A CONST: use clang-tidy to mark local variables const where possible Mar 25, 2026

cameel added the refactor label Mar 25, 2026

msooseth changed the title ~~You get a const, you get a const, EVERYBODY GETS A CONST: use clang-tidy to mark local variables const where possible~~ Use clang-tidy to add const wherever possible Mar 25, 2026

msooseth force-pushed the gimme-const branch from f208580 to b1f7bd1 Compare April 1, 2026 12:00