Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator #182

denishug · 2024-09-25T21:08:10Z

No description provided.

- Fixe #924, #925, #989 - add tests case for power operator

libcob/intrinsic.c

engboris · 2024-09-26T17:34:24Z

Hello @denishug and thank you for your contribution.

It is usually requested that contributors add an entry to ChangeLog files when modifying the source code. Here, I believe libcob/ChangeLog must be modified. There is a strict formatting (number of spaces, difference between space and tab).

Example in our PR #191 (merged)

Update Changelog

engboris · 2024-10-21T12:15:46Z

libcob/ChangeLog

@@ -1,4 +1,13 @@

+2024-09-26  Denis Hugonnard-Roce <[email protected]>


Typo in the name

engboris · 2024-10-21T12:16:02Z

libcob/ChangeLog

+	* intrinsic.c (cob_decimal_pow) Correct c++ comment style to C
+	comment style
+
+2024-09-25  Denis Hugonnard-Roce <[email protected]>


Typo in the name

engboris · 2024-10-21T12:23:31Z

libcob/intrinsic.c

+	/* Fix  #925 : Avoid GMPLIB CRASH */
+
+	mpf_set(cob_mpft3,cob_mpft) ;
+	if ( sign_nbr == -1) {


Remove space before condition

engboris · 2024-10-21T12:24:32Z

libcob/intrinsic.c

+
+ Process_case_0:
+	if (sign_nbr == 0) {
+		if ( sign_exp == 1) {


Remove space before condition

engboris · 2024-10-21T12:28:48Z

libcob/intrinsic.c

-		if (sign == 0) {
-			/* 0 ^ 0 */
-			cob_set_exception (COB_EC_SIZE_EXPONENTIATION);
+	sign_nbr = mpz_sgn(pd1->value);


In order to strictly follow GnuCOBOL's coding style, a space character should be inserted before function calls. And it shouldn't occur before the ; character.

For reference, don't hesitate to look at how the code is written elsewhere in GnuCOBOL's source.

GitMensch

Thanks for this nice piece of work. I think the Changelog entries could be improved and there are some code parts + formatting, but overall quite good!

GitMensch · 2024-10-21T12:20:12Z

ChangeLog

As those changes are all below libcob, they don't belong here - just revert that file, moving the better text there.
Since quite some time we only Changlog the testsuite if there are "bigger" changes (moving tests around or adding new internal features), so you can drop that part.

GitMensch · 2024-10-21T12:22:39Z

libcob/ChangeLog

+	* intrinsic.c (cob_decimal_pow) Correct c++ comment style to C
+	comment style
+
+2024-09-25  Denis Hugonnard-Roce <[email protected]>


as this is an update to a not-yet-checked in change, just edit the date for the original change and drop the update

GitMensch · 2024-10-21T12:24:11Z

tests/testsuite.src/run_fundamental.at

+AT_CHECK([$COMPILE prog.cob], [0], [], [])
+AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [], [])
+AT_CLEANUP
+


minor beautify: please separate test cases with a double empty line - also above

GitMensch · 2024-10-21T12:26:12Z

tests/testsuite.src/run_fundamental.at

+AT_CLEANUP
+
+AT_SETUP([Power size error and limits cases 2])
+AT_KEYWORDS([POWER])


Words that are in AT_SETUP are automatically added to the keyword list. Replacing it with the main statement - here COMPUTE - would be useful (also applies above).

GitMensch · 2024-10-21T12:28:32Z

tests/testsuite.src/run_fundamental.at

please add an m4 comment about which tests are for which bug report - this provides more context and is also useful in the future

GitMensch · 2024-10-21T12:31:18Z

libcob/intrinsic.c

+		break;
+
+	case 0:
+		goto Process_case_0;


there is a single case for this - please inline instead of using goto

GitMensch · 2024-10-21T12:32:51Z

libcob/intrinsic.c

-		mpz_set_ui (pd1->value, 1UL);
-		pd1->scale = 0;
-		return;
+		goto Compute_Power;


please reorder that case to have zero first, this can then return while the fall-through of 1/-1 is easier to follow and we don't need the Compute_Power label any more

GitMensch · 2024-10-21T12:33:38Z

libcob/intrinsic.c

+	sign_nbr = mpz_sgn(pd1->value);
+	sign_exp = mpz_sgn(pd2->value);
+
+	power_case = sign_nbr * sign_exp;
+
+	cob_trim_decimal(pd2);
+	cob_trim_decimal(pd1);


Can you please reformat to use a space before the parenthesis (also applies to other places)?

GitMensch · 2024-10-21T12:42:21Z

libcob/intrinsic.c

+	sign_nbr = mpz_sgn(pd1->value);
+	sign_exp = mpz_sgn(pd2->value);
+
+	power_case = sign_nbr * sign_exp;


Those are all "cheap" and don't break for NAN scale - please const those three variables directly at the start of the function.

GitMensch · 2024-10-21T12:44:03Z

libcob/intrinsic.c

-		if (mpz_sgn (pd2->value) == -1
-		 && mpz_fits_slong_p (pd2->value)) {
+
+		if (mpz_sgn (pd2->value) == -1 ) {


Isn't that sign_exp now?

Recode cob_decimal_pow function

85b1770

- Fixe #924, #925, #989 - add tests case for power operator

engboris requested a review from GitMensch September 26, 2024 08:57

lefessan reviewed Sep 26, 2024

View reviewed changes

libcob/intrinsic.c Show resolved Hide resolved

engboris requested review from engboris and removed request for engboris September 26, 2024 17:22

denishug added 2 commits September 26, 2024 21:49

20240925 Correct libcob/intrinsic.c code to compile on mac os

ba09a87

Update Changelog

2024-09-27 Fix C style comment in intrinsic.c (cob_decimal_pow function)

6ce83a5

engboris reviewed Oct 21, 2024

View reviewed changes

libcob/ChangeLog

@@ -1,4 +1,13 @@

2024-09-26 Denis Hugonnard-Roce <[email protected]>

Copy link

Contributor

engboris Oct 21, 2024

Choose a reason for hiding this comment

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

Typo in the name

engboris reviewed Oct 21, 2024

View reviewed changes

GitMensch requested changes Oct 21, 2024

View reviewed changes

engboris changed the title ~~Recode cob_decimal_pow function - Fixe #924, #925, #989 - add tests case for power operator~~ Recode cob_decimal_pow function - Fix #924, #925, #989 - add tests case for power operator Oct 21, 2024

engboris changed the title ~~Recode cob_decimal_pow function - Fix #924, #925, #989 - add tests case for power operator~~ Recode cob_decimal_pow function - Fix for #924, #925, #989 - add tests case for power operator Oct 21, 2024

engboris changed the title ~~Recode cob_decimal_pow function - Fix for #924, #925, #989 - add tests case for power operator~~ Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator #182

Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator #182

denishug commented Sep 25, 2024

engboris commented Sep 26, 2024

engboris Oct 21, 2024

engboris Oct 21, 2024

engboris Oct 21, 2024

engboris Oct 21, 2024

engboris Oct 21, 2024 •

edited

Loading

GitMensch left a comment

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

GitMensch Oct 21, 2024

		@@ -1,4 +1,13 @@

		2024-09-26 Denis Hugonnard-Roce <[email protected]>

Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator #182

Are you sure you want to change the base?

Recode cob_decimal_pow function - Fix for #924, #925, #989 - add test cases for power operator #182

Conversation

denishug commented Sep 25, 2024

engboris commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engboris Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engboris Oct 21, 2024 •

edited

Loading