Skip to content

Conversation

@firewave
Copy link
Collaborator

No description provided.

@firewave firewave marked this pull request as ready for review October 19, 2025 23:19
@sonarqubecloud
Copy link

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

I don't know.. of course I want to have performance. But we obfuscate the code in various places that are probably not hotspots.
I feel that the original code is more well written and readable.

checkRecursive(expr, startEndTokens.first, startEndTokens.second->next(), exprVarIds, local, true, depth);
if (result.type != Result::Type::NONE)
return result;
{
Copy link
Owner

Choose a reason for hiding this comment

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

this is quite pedantic by clang imho. FwdAnalysis::Result is small and simple data. But yes if we want to turn on the warning then you have to fix all warnings so I guess there's not much we can do.

return "0.0";
if (s.find_first_of(".e") == std::string::npos)
return s + ".0";
s = "0.0";
Copy link
Owner

Choose a reason for hiding this comment

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

hmm I like the old code better here from a readability point of view.

result = execute(elseStart->scope());
} else {
return {unknown()};
result = {unknown()};
Copy link
Owner

Choose a reason for hiding this comment

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

it is less obvious what happens here. if the loop continues then other results can be returned.. so I want to see an unconditional return.

overflow = true;
indexValues.push_back(values.front());
}
if (overflow)
Copy link
Owner

Choose a reason for hiding this comment

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

this looks more weird imho.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants