Factor out computation of maximum union member #5251
Conversation
tautschnig
requested review from
chrisr-diffblue,
kroening,
peterschrammel and
smowton
as code owners
| /// \return Pair of a member_exprt pointing to the maximum fixed bit-width | ||
| /// member of \p union_type and the bit width of that member. | ||
| static optionalt<std::pair<member_exprt, mp_integer>> | ||
| find_maximum_union_component( |
There was a problem hiding this comment.
Fixed (and left-hand sides are renamed accordingly).
| return {}; | ||
| else | ||
| return std::make_pair( | ||
| member_exprt{union_expr, max_comp_name, max_comp_type}, max_width); |
There was a problem hiding this comment.
A bit surprising that a utility would make a member-expression, rather than just returning the widest component
There was a problem hiding this comment.
Good call - this makes one use slightly more work (having to construct the member_exprt in place), but avoids a parameter that isn't really necessary and could be used in a wrong way.
Avoids duplicate code and enables future re-use.
tautschnig
force-pushed
the
factor-out-max-member
branch
from
c5b03fc to
b44360c
Compare
tautschnig
assigned tautschnig and unassigned smowton, kroening, peterschrammel and chrisr-diffblue
| /// \return Pair of a componentt pointing to the maximum fixed bit-width | ||
| /// member of \p union_type and the bit width of that member. | ||
| static optionalt<std::pair<struct_union_typet::componentt, mp_integer>> | ||
| find_widest_union_component(const union_typet &union_type, const namespacet &ns) |
There was a problem hiding this comment.
Is there any particular reason this returns an optional pair, rather than just an optional componentt ? Given that the componentt has the the appropriate type in it and the caller could just do pointer_offset_bits themselves. If it's just to simplify future clients, then fine :-) But just wondering as I don't see any current clients using the second half of the pair, and having the extra ->first or ->second slightly reduces readability at the call sites.
There was a problem hiding this comment.
Yes, that second part is only used in #5252. I hope that's ok - I'll go ahead and merge.
There was a problem hiding this comment.
FWIW, you're of course right that the caller could compute the size themselves, but I thought duplicating work was not a good idea.
There was a problem hiding this comment.
Fine by me. I was reviewing in PR order so hadn't seen the followup PR at that point.
Avoids duplicate code and enables future re-use.
This is in preparation of further union support in byte-extract lowering. Refactoring is tested via existing unit tests.