scottmcm opened PR #2953 from add-memcmp
to main
:
The comment says the enum is "likely to grow" and the function's been in libc since C89, so hopefully this is ok.
I'd like to use it for emitting things like array equality.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
scottmcm edited PR #2953 from add-memcmp
to main
:
This adds
LibCall::Memcmp
and a correspondingFunctionBuilder::call_memcmp
, following the existing patterns ofmemcpy
,memset
, &memmove
. The comment says the enum is "likely to grow" and thememcmp
's been in libc since C89, so hopefully this is ok.I'd like to have this for emitting equality tests of large, simple no-padding types, like
[u32; 123]
.I don't know who should review this.
scottmcm updated PR #2953 from add-memcmp
to main
.
scottmcm updated PR #2953 from add-memcmp
to main
.
scottmcm updated PR #2953 from add-memcmp
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
This could probably become a helper on
Type
, perhapsType::int_with_byte_size(...)
(and create a correspondingType::int_with_bit_size(...)
and maybe thefloat
andbool
equivalents too).
scottmcm submitted PR review.
scottmcm created PR review comment:
Hmm,
Type::int
is already mostlyType::int_with_bit_size
, just with a different parameter type.Here's a few possible directions. Let me know which you'd prefer I do, @cfallin
- Add
Type::int_with_byte_size
andType::int_with_bit_size
, concentrating on the "different integer width" use by having them takeimpl TryInto<u16>
.- Add a
Type::int_bytes
that takesu16
- Rename
Type::int
toType::int_bits
and addType::int_bytes
(rg only spots 10 obvious uses ofType::int
so it might not be as horrible as I'd have guessed)- One of the above, but using a type other than
u16
forType::int
(TBHu16
is kinda odd there, since the biggest value it cares about is 128 and would fit in au8
, but it's also not theu32
that rust'sstd
tends to use for numbers of bits)- Some combination thereof
- Make a follow-up PR for one of the above
- Do nothing
cfallin submitted PR review.
cfallin created PR review comment:
Ah, I had missed the existing
Type::int()
-- IMHO, it might be better to leave that as-is (Type::int(32)
is simple/ergonomic/very clear) and then add this asType::int_with_byte_size()
(I slightly prefer this overint_bytes
, the latter sounds like it's a special kind of int that's a vector of bytes or something like that). And I guess let's match theu16
for consistency (I'm not sure whyu16
, maybe to allow 256-bit vecs but still be somewhat compact?).Thanks!
scottmcm submitted PR review.
scottmcm created PR review comment:
I'm not sure why u16, maybe to allow 256-bit vecs but still be somewhat compact?
That would make sense if it was stored, but
Type
is justpub struct Type(u8);
, so it's just a parameter, and thus the choice seems like it could be pretty much anything.I'll go add
Type::int_with_byte_size(u16) -> Type
as requested.
scottmcm updated PR #2953 from add-memcmp
to main
.
scottmcm updated PR #2953 from add-memcmp
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
OK, yeah, that existing bit is unclear then; but, better saved for a later cleanup :-) Thanks!
cfallin submitted PR review.
cfallin merged PR #2953.
Last updated: Dec 23 2024 at 13:07 UTC