Skip to content

Conversation

@bbannier
Copy link
Member

@bbannier bbannier commented Dec 17, 2025

I looked into this since I saw a write: [i] in the CFG in #2236. This fixes a small issue, but there is definitely more we could do here.

When computing the CFG data access information we would previously
assume that any operator use would both read and write its operands,
e.g., we would assume that an equality check could write its operands.
Due to how we resolve operators of non-const operands this does reflect
what we see in many ASTs, but does not take into account when we
actually use an operand as const.

This patch adds handling of operators so that we can avoid recording
writes when the operand is used as const.
@bbannier bbannier self-assigned this Dec 17, 2025
@bbannier bbannier moved this to In Progress in Spicy projects Dec 17, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 17, 2025

CodSpeed Performance Report

Merging #2238 will not alter performance

Comparing topic/bbannier/cfg-const-operands (7771551) with main (d3fe866)

Summary

✅ 28 untouched

Comment on lines +825 to +826
else if ( auto* operator_ = node->tryAs<expression::ResolvedOperator>();
operator_ && isConstOperand(*operator_, *name) )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-const uses we still fall through to the else branch which also sets the node keep.

Ideally we'd handle all operators here, always set read and write for non-const uses. We still would need extra handling for member-like operators which could have side effects (definitely MemberCall, possibly Member and TryMember).

Copy link
Member

@rsmmr rsmmr Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to look at the operator's signature: all the method-like operators have a self with a Kind that's either In or InOut. And generally, the signatures are good to look at for all operands I think: In is read-only, InOut means operator can modify the operand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants