Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xmerl: replace size/1 by xxx_size/1 #6796

Conversation

kikofernandez
Copy link
Contributor

The size/1 BIF is not optimized by the JIT, and its use can result in worse types for Dialyzer.

When one knows that the value being tested must be a tuple, tuple_size/1 should always be preferred.

When one knows that the value being tested must be a binary, byte_size/1 should be preferred. However, byte_size/1 also accepts a bitstring (rounding up size to a whole number of bytes), so one must make sure that the call to byte_size/ is preceded by a call to is_binary/1 to ensure that bitstrings are rejected. Note that the compiler removes redundant calls to is_binary/1, so if one is not sure whether previous code had made sure that the argument is a binary, it does not harm to add an is_binary/1 test immediately before the call to byte_size/1.

This PR replaces #6783 due to the name branching convention. Apart from that, they are identical in content.

@kikofernandez kikofernandez added team:PS Assigned to OTP team PS enhancement testing currently being tested, tag is used by OTP internal CI labels Feb 6, 2023
@kikofernandez kikofernandez added this to the OTP-26.0-rc1 milestone Feb 6, 2023
@kikofernandez kikofernandez self-assigned this Feb 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

CT Test Results

       2 files       24 suites   6m 13s ⏱️
2 154 tests 2 146 ✔️   8 💤 0
3 970 runs  3 936 ✔️ 34 💤 0

Results for commit f0b6beb.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

lib/xmerl/src/xmerl_eventp.erl Outdated Show resolved Hide resolved
The <c>size/1</c> BIF is not optimized by the JIT, and its use can
result in worse types for Dialyzer.

When one knows that the value being tested must be a tuple,
<c>tuple_size/1</c> should always be preferred.

When one knows that the value being tested must be a binary,
<c>byte_size/1</c> should be preferred. However, <c>byte_size/1</c> also
accepts a bitstring (rounding up size to a whole number of bytes), so
one must make sure that the call to <c>byte_size/</c> is preceded by a
call to <c>is_binary/1</c> to ensure that bitstrings are rejected. Note
that the compiler removes redundant calls to <c>is_binary/1</c>, so if
one is not sure whether previous code had made sure that the argument is
a binary, it does not harm to add an <c>is_binary/1</c> test immediately
before the call to <c>byte_size/1</c>.
@kikofernandez kikofernandez force-pushed the kiko/xmerl/replace-size-by-xxx_size/GH-6672/OTP-18432 branch from 35956e2 to f0b6beb Compare February 6, 2023 12:16
@kikofernandez kikofernandez merged commit 3dcb1d3 into erlang:maint Feb 6, 2023
@kikofernandez kikofernandez deleted the kiko/xmerl/replace-size-by-xxx_size/GH-6672/OTP-18432 branch February 6, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants