GH-251 (revert): remove coercion-to-signed-integer for |x| expressions

For `|x|`, where `x` is an expression with an integral result, an
implicit coercion of that result into signed `int` type no longer takes
place.

This was actually the behavior before Zeek 3.0 as well, but the attempt
to prevent mistakes that easily result from integer literals in Zeek
being unsigned like `|5 - 9|` causing an overflow/wraparound and
yielding a very large number is not generally consistent since overflows
are still generally able to happen in other ways and also in other
contexts besides just absolute-values.  So the preference was to revert
to a behavior that favors consistency.  For reference, see
https://github.com/zeek/zeek/pull/251#issuecomment-713956976
This commit is contained in:
Jon Siwek 2020-10-22 16:56:37 -07:00
parent 22ef67888c
commit 73c1af838c
5 changed files with 22 additions and 8 deletions

11
NEWS
View file

@ -98,6 +98,17 @@ Changed Functionality
not pre-configured to listen for incoming connections from other cluster not pre-configured to listen for incoming connections from other cluster
nodes. nodes.
- The ``|x|`` operator, where ``x`` is an expression with an integral result,
no longer performs an implicit coercion of that result into a signed
``int`` type. This was actually the behavior before Zeek 3.0 as well, but
the attempt to prevent mistakes that easily result from integer literals in
Zeek being unsigned like ``|5 - 9|`` causing an overflow/wraparound and
yielding a very large number is not generally consistent since overflows
are still generally able to happen in other ways and also in other contexts
besides just the absolute-value operator. So the preference was to revert
to a behavior that favors consistency. For reference, see
https://github.com/zeek/zeek/pull/251#issuecomment-713956976
Removed Functionality Removed Functionality
--------------------- ---------------------

2
doc

@ -1 +1 @@
Subproject commit c5c73eaf52d481cee6ff180f1aa6edb75eb11e9e Subproject commit 36c1716769917deed461b4cc80dfb07546d4ace3

View file

@ -767,10 +767,6 @@ expr:
{ {
zeek::detail::set_location(@1, @3); zeek::detail::set_location(@1, @3);
zeek::detail::ExprPtr e{zeek::AdoptRef{}, $2}; zeek::detail::ExprPtr e{zeek::AdoptRef{}, $2};
if ( zeek::IsIntegral(e->GetType()->Tag()) )
e = zeek::make_intrusive<zeek::detail::ArithCoerceExpr>(std::move(e), zeek::TYPE_INT);
$$ = new zeek::detail::SizeExpr(std::move(e)); $$ = new zeek::detail::SizeExpr(std::move(e));
} }

View file

@ -1,8 +1,10 @@
### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
IPv4 Address 1.2.3.4: 32 IPv4 Address 1.2.3.4: 32
IPv6 Address ::1: 128 IPv6 Address ::1: 128
Boolean T: 1 Boolean T: 1
Count 10: 10 Count 10: 10
Expr: 4 Expr: 18446744073709551612
Signed Expr: 4
Double -1.23: 1.230000 Double -1.23: 1.230000
Enum ENUM3: 2 Enum ENUM3: 2
File 21.000000 File 21.000000

View file

@ -64,9 +64,14 @@ print fmt("Boolean %s: %d", b, |b|);
# Size of count: identity. # Size of count: identity.
print fmt("Count %s: %d", c, |c|); print fmt("Count %s: %d", c, |c|);
# Size of integral arithmetic expression should coerce to int before absolute # Integer literals that lack a "+" or "-" modifier are of the unsigned "count"
# value operation to help prevent common unsigned int overflow situations. # type, so this wraps to a very large number. It may be more intuitive if it
# were to coerce to a signed integer, but it can also be more favorable to
# simply have consistent behavior across arbitrary arithmetic expressions even
# if that may result in occassional, unintended overflow/wrapping.
print fmt("Expr: %d", |5 - 9|); print fmt("Expr: %d", |5 - 9|);
# Same arithmetic on signed integers is likely what's originally intended.
print fmt("Signed Expr: %d", |+5 - +9|);
# Size of double: returns absolute value. # Size of double: returns absolute value.
print fmt("Double %s: %f", d, |d|); print fmt("Double %s: %f", d, |d|);