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.

Sign up for GitHub

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

Use decnum for addition #1997

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Use decnum for addition #1997

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
05e5984
Use decnum to add numbers
wtlangford Oct 22, 2019
2e51cf5
Clean up context creation
wtlangford Oct 22, 2019
11de034
fixup! Use decnum to add numbers
wtlangford Oct 22, 2019
08d6dec
fixup! Use decnum to add numbers
wtlangford Oct 23, 2019
d812abb
fixup! Use decnum to add numbers
wtlangford Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/builtin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,7 @@ static jv f_plus(jq_state *jq, jv input, jv a, jv b) {
jv_free(b);
return a;
} else if (jv_get_kind(a) == JV_KIND_NUMBER && jv_get_kind(b) == JV_KIND_NUMBER) {
jv r = jv_number(jv_number_value(a) +
jv_number_value(b));
jv_free(a);
jv_free(b);
return r;
return jv_number_add(a, b);
} else if (jv_get_kind(a) == JV_KIND_STRING && jv_get_kind(b) == JV_KIND_STRING) {
return jv_string_concat(a, b);
} else if (jv_get_kind(a) == JV_KIND_ARRAY && jv_get_kind(b) == JV_KIND_ARRAY) {
Expand Down
68 changes: 48 additions & 20 deletions src/jv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,28 +230,24 @@ static decContext* tsd_dec_ctx_get(pthread_key_t *key) {
return ctx;
}

decContext _ctx = {
0,
DEC_MAX_EMAX,
DEC_MIN_EMAX,
DEC_ROUND_HALF_UP,
0, /*no errors*/
0, /*status*/
0, /*no clamping*/
};
ctx = malloc(sizeof(decContext));
if (!ctx) {
return ctx;
}

// Initialize the context, clear any traps
decContextDefault(ctx, DEC_INIT_BASE);
ctx->traps = 0;

if (key == &dec_ctx_key) {
_ctx.digits = DEC_MAX_DIGITS;
ctx->digits = DEC_MAX_DIGITS;
} else if (key == &dec_ctx_dbl_key) {
_ctx.digits = BIN64_DEC_PRECISION;
ctx->digits = BIN64_DEC_PRECISION;
}

ctx = malloc(sizeof(decContext));
if (ctx) {
*ctx = _ctx;
if (pthread_setspecific(*key, ctx) != 0) {
fprintf(stderr, "error: cannot store thread specific data");
abort();
}
if (pthread_setspecific(*key, ctx) != 0) {
fprintf(stderr, "error: cannot store thread specific data");
abort();
}
return ctx;
}
Expand Down Expand Up @@ -291,8 +287,9 @@ static decNumber* jvp_dec_number_ptr(jv j) {

static jvp_literal_number* jvp_literal_number_alloc(unsigned literal_length) {

/* The number of units needed is ceil(DECNUMDIGITS/DECDPUN) */
int units = ((literal_length+DECDPUN-1)/DECDPUN);
// The number of units needed is ceil(DECNUMDIGITS/DECDPUN)
// We already have one in the decNumber, so we subtract one from that
int units = ((literal_length+DECDPUN-1)/DECDPUN)-1;

jvp_literal_number* n = jv_mem_alloc(
sizeof(jvp_literal_number)
Expand All @@ -314,6 +311,7 @@ static jv jvp_literal_number_new(const char * literal) {

if (ctx->status & DEC_Conversion_syntax) {
jv_mem_free(n);
decContextClearStatus(ctx, DEC_Conversion_syntax);
return JV_INVALID;
}

Expand Down Expand Up @@ -484,6 +482,36 @@ int jvp_number_cmp(jv a, jv b) {
}
}

jv jv_number_add(jv a, jv b) {
if (jvp_number_is_literal(a) && jvp_number_is_literal(b)) {
decNumber *da = jvp_dec_number_ptr(a);
decNumber *db = jvp_dec_number_ptr(b);

unsigned digits = MAX(da->digits+da->exponent, db->digits+db->exponent) -
MIN(da->exponent, db->exponent) + 1;
jvp_literal_number *nlit = jvp_literal_number_alloc(digits);
nlit->refcnt = JV_REFCNT_INIT;
nlit->literal_data = NULL;
nlit->num_double = NAN;

decContext *ctx = DEC_CONTEXT();
decNumberAdd(&nlit->num_decimal, da, db, ctx);
decNumberReduce(&nlit->num_decimal, &nlit->num_decimal, ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we believe that the operation above (the add) is able to fit the result into our calculated digits then the reduce should not be needed

Copy link
Contributor

@leonid-s-usov leonid-s-usov Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After performing some tests I can tell that decNumberAdd will function correctly with our digits calculation and the only thing we should do after is decNumberTrim. We shouldn't call decNumberReduce as it will convert the number to its minimal scientific form which is not desirable IMO:

Using reduce:
61e2 + 00009876543210000000e-2 => 9.87654321061E+13

Using trim:
61e2 + 00009876543210000000e-2 => 98765432106100

Not using either:
61e2 + 00009876543210000000e-2 => 98765432106100.00

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the decNumberReduce was meant to address the trailing zero issue- I wasn't concerned about decNumberAdd functioning properly. decNumberTrim is the best option, I'll swap to that.

// TODO: Check context for error?

jv_free(a);
jv_free(b);

jv r = {JVP_FLAGS_NUMBER_LITERAL, 0, 0, JV_NUMBER_SIZE_INIT, {&nlit->refcnt}};
return r;
}

jv r = jv_number(jv_number_value(a) + jv_number_value(b));
jv_free(a);
jv_free(b);
return r;
}

/*
* Arrays (internal helpers)
*/
Expand Down
1 change: 1 addition & 0 deletions src/jv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ jv jv_number(double);
jv jv_number_with_literal(const char*);
double jv_number_value(jv);
int jv_is_integer(jv);
jv jv_number_add(jv a, jv b);

int jv_number_has_literal(jv n);
const char* jv_number_get_literal(jv);
Expand Down
2 changes: 1 addition & 1 deletion src/parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ static block constant_fold(block a, block b, int op) {
int cmp = jv_cmp(jv_a, jv_b);

switch (op) {
case '+': res = jv_number(na + nb); break;
case '+': res = jv_number_add(jv_copy(jv_a), jv_copy(jv_b)); break;
case '-': res = jv_number(na - nb); break;
case '*': res = jv_number(na * nb); break;
case '/': res = jv_number(na / nb); break;
Expand Down
2 changes: 1 addition & 1 deletion src/parser.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ static block constant_fold(block a, block b, int op) {
int cmp = jv_cmp(jv_a, jv_b);

switch (op) {
case '+': res = jv_number(na + nb); break;
case '+': res = jv_number_add(jv_copy(jv_a), jv_copy(jv_b)); break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it too late to copy the arguments here? jv_cmp above has already consumed those, hasn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it has. I'll fix that.

case '-': res = jv_number(na - nb); break;
case '*': res = jv_number(na * nb); break;
case '/': res = jv_number(na / nb); break;
Expand Down
7 changes: 6 additions & 1 deletion tests/jq.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,8 +1698,13 @@ map(. == 1)
null
false

# Applying addition to the value will not truncate the result to double

# Applying arithmetic to the value will truncate the result to double
. + 1
418502930602131457
418502930602131458

# Applying other arithmetic to the value will truncate the result to double

. - 10
13911860366432393
Expand Down