On 03.03.26 19:42, Peter Xu wrote:
- errp);
- if (ret) {
- return ret;
- }
+ } else if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
+ return -EINVAL;
}
I understand you may prefer merging them whenever possible, but for things
like this personally I normally keep "else" and "if" separate:
if (!se->vmsd) {
vmstate_save_old_style(f, se, vmdesc);
} else {
if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
return -EINVAL;
}
}
"if" / "else if" can hid problems when we grow the condition in the future
(I recall we hit one when reviewing the other series adding the _errp()
variances). That doesn't apply here but split "if" and "else" also helps
slightly on readabilty for me,
Hmm I thought about that. But it seemed strange to me to keep
} else {
if () {
}
}
Now looking at the code after a while, first thought is "o, I'm doing something
wrong removing "else" branch, some cases may be lost!", some extra time is
needed to understand it.
What's not pretty with my code is that when we have some
if () {}
else if () {}
else if () {}
...
We expect that conditions are of the same type, and actions are in corresponding
{} bodies - a kind of smart switch-case. But this final "else if" has absolutely
another type of condition: it's actually an action. And that's why it's hard to
read it for me.
I'll follow your suggestion.
so it says: there's no priority of doing
old_style or vmsd_style, it's just that we support both with/without vmsd
pointer, and it shows clearly what we do on which path.
I would expect the compiler will always generate the same byte code, but I
didn't check.
No strong feelings.
--
Best regards,
Vladimir
