Add suport to is_integer/3 BIF (and to OTP-29)#2150
Conversation
|
|
looks good:
|
79189c9 to
42a6fb1
Compare
Thanks, everything fixed. |
|
https://ampcode.com/threads/T-019ca09c-de95-774e-b3bb-bd023fcf800f PR #2150 Review:
|
| # | File | Line | Severity | Issue |
|---|---|---|---|---|
| 1 | libs/jit/src/opcodes.hrl |
171 | Medium | OP_BIF3 (185) added to opcodes.h but missing from opcodes.hrl. Comment says "keep this list in sync". JIT won't handle the new opcode. |
| 2 | src/libAtomVM/opcodesswitch.h |
7733 | Medium | Copy-paste error: trace message says "bif2/2" but should be "bif3/3" in the OP_BIF3 handler. |
| 3 | src/libAtomVM/bif.c |
259 | Medium | UNUSED(fail_label) declared but fail_label is used by RAISE_ERROR_BIF on line 274. Misleading and may cause warnings. |
| 4 | CHANGELOG.md |
83 | Low | Typo: "OPT-29" should be "OTP-29". |
and the formatting of course - otherwise good!
940180c to
1200210
Compare
Fixed everything. Thanks for reporting this. |
| * @brief Compare two signed multi-precision integers | ||
| * | ||
| * Compares two multi-precision integers with explicit sign information. | ||
| * Magnitudes are compared using intn_cmp(), which ignores sign and tolerates |
There was a problem hiding this comment.
llm strongly insists intn_cmp() should be intn_cmpu() here - ?
| avm_int64_t b = term_maybe_unbox_int64(arg3); | ||
| return (a <= n) && (n <= b) ? TRUE_ATOM : FALSE_ATOM; | ||
| } else { | ||
| if ((!term_is_any_integer(arg2) || !term_is_any_integer(arg3))) { |
There was a problem hiding this comment.
redundant outer parentheses..
petermm
left a comment
There was a problem hiding this comment.
absolute nitpicks left - looking good:
PR #2150 Review (v3): is_integer/3 BIF (OTP-29)
Commit: 1200210e6
Summary
The maintainer addressed all previous medium/high-severity issues:
- ✅
opcodes.hrlnow includesOP_BIF3and updatedOPCODE_MAX - ✅ Trace message fixed to
"bif3/6" - ✅
UNUSED(fail_label)removed (it's used byRAISE_ERROR_BIF) - ✅
UNUSED(ctx)correctly omitted (ctxis used insideRAISE_ERROR_BIFmacro) - ✅ Test exports moved inside
-ifdef(OTP29_OR_LATER) - ✅ JIT support added in
jit.erlwithOP_BIF3handler - ✅ CI matrix extended with OTP-29 JIT build
- ✅
test_errors/0now covers non-integer Term + non-integer bounds →badarg - ✅
"OPT-29"typo in CHANGELOG fixed to"OTP-29"
Remaining Issues
| # | File | Line | Severity | Category | Issue |
|---|---|---|---|---|---|
| 1 | src/libAtomVM/intn.h |
283 | Low | Doc | Confirmed (oracle): Stale docstring: "Magnitudes are compared using intn_cmp()" should be "intn_cmpu()" — the unsigned function was renamed but the doc for intn_cmp (signed) still references the old name. Not a self-reference; the implementation (intn.c:468) calls intn_cmpu(). |
| 2 | src/libAtomVM/bif.c |
274 | Low | Style | return FALSE_ATOM; uses 2-space indent (10 spaces) but surrounding code uses 4-space indent (12 spaces). See line 271 RAISE_ERROR_BIF for the expected indentation level. |
| 3 | src/libAtomVM/bif.c |
270 | Low | Style | Redundant outer parentheses: `if ((!term_is_any_integer(arg2) |
Notes
- The opcode gap (183–184 skipped,
OP_BIF3= 185) is correct — opcodes must match OTP's BEAM format numbering. - The fast paths (small-int, int64) correctly fall through to the
elseblock when any argument is non-integer, where proper validation occurs. RAISE_ERROR_BIFimplicitly usesctxvia macro expansion (lines 55–56), so thectxparameter is genuinely needed despite appearing unused at the source level.intn_cmpsigned comparison logic is correct: sign mismatch → immediate result; same sign → unsigned magnitude compare, negated if both negative.- Test coverage is comprehensive: small ints, int64, bigints, mixed types, guard context, direct call context, swapped intervals, single-point intervals, non-integer terms, non-integer bounds, and combined non-integer term + bounds.
OTP-29 introduces is_integer/3 BIF, so in order to support it we have introduced it as well. - bif.c implements the new BIF. It checks first small integers in order to provide best performance with them. - opcodesswitch.h: has been updated to support the new opcode for calling BIFs with 3 arguments, such as `is_integer/3`. - jit.erl likewise. Also now JIT is tested also with OTP-29. - intn has been changed in order to provide a helper for signed big integers comparison (now we have `intn_cmpu` and `intn_cmp`). - README and documentation has been updated with OTP-29 support. Signed-off-by: Davide Bettio <davide@uninstall.it>
OTP-29 introduces is_integer/3 BIF, so in order to support it we have introduced it as well.
bif.c implements the new BIF. It checks first small integers in order to provide best performance with them.
opcodesswitch.h: has been updated to support the new opcode for calling BIFs with 3 arguments, such as
is_integer/3.intn has been changed in order to provide a helper for signed big integers comparison (now we have
intn_cmpuandintn_cmp).README and documentation has been updated with OTP-29 support.
Closes #2114
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later