After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764663 - Add abs() to double type
Add abs() to double type
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Basic Types
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-05 23:25 UTC by oliver.steven
Modified: 2016-04-14 07:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (677 bytes, patch)
2016-04-05 23:25 UTC, oliver.steven
none Details | Review
Updated patch (654 bytes, patch)
2016-04-09 15:42 UTC, oliver.steven
none Details | Review
Updated patch (713 bytes, patch)
2016-04-10 13:51 UTC, oliver.steven
rejected Details | Review
glib-2.0: Add abs() to float and double (1.04 KB, patch)
2016-04-12 13:13 UTC, Rico Tzschichholz
none Details | Review
glib-2.0: Add abs() to float and double (1.04 KB, patch)
2016-04-13 14:51 UTC, Rico Tzschichholz
committed Details | Review

Description oliver.steven 2016-04-05 23:25:26 UTC
Created attachment 325456 [details] [review]
Patch

Use the C functions fabs() to get the abs of a double in Vala.
Comment 1 Al Thomas 2016-04-06 10:09:41 UTC
Why the need for cheader_filename = "stdlib.h"?

As far as I can tell fabs is part of math.h, which is already included in the VAPI.

The C standard doesn't appear to be publically available, but Posix is:
http://pubs.opengroup.org/onlinepubs/7908799/xsh/fabs.html
Comment 2 oliver.steven 2016-04-09 15:42:30 UTC
Created attachment 325640 [details] [review]
Updated patch

Updated patch based upon reviewer comments
Comment 3 Al Thomas 2016-04-09 16:31:10 UTC
Thanks for the update, looks good. For such a trivial patch I'm reluctant to point out it says [Patch 2/2]. If you have a moment and submit another then also in the GLib-2.0 bindings there appear to be groupings of bindings for each simple type, e.g. abs() for gshort is with the min(), max() and clamp() functions. Might be an idea to group abs for gdouble with min() and max() as well.
Comment 4 oliver.steven 2016-04-10 13:51:08 UTC
Created attachment 325666 [details] [review]
Updated patch

Updated based on reviewer comments
Comment 5 Al Thomas 2016-04-11 08:52:20 UTC
Review of attachment 325666 [details] [review]:

Patch applies and works. Job done. Thanks for your perseverance and attention to detail.
Comment 6 oliver.steven 2016-04-11 23:57:30 UTC
You're welcome. Thank you for being patient and kind about it!
Comment 7 Rico Tzschichholz 2016-04-12 13:13:00 UTC
Created attachment 325800 [details] [review]
glib-2.0: Add abs() to float and double
Comment 8 Al Thomas 2016-04-12 21:49:21 UTC
Review of attachment 325800 [details] [review]:

Thanks for adding abs() for float as well, but both fabs() and fabsf() are defined in math.h.
I know the C function abs() is defined in stdlib.h and maybe that is where the confusion comes
from.

From the GNU C library documentation, http://www.gnu.org/software/libc/manual/html_node/Absolute-Value.html , 
"Prototypes for abs, labs and llabs are in stdlib.h; imaxabs is declared in inttypes.h; fabs, fabsf and fabsl are declared in math.h"

I marked the first accepted patch as commit_now so it could be picked up and committed, but as 
adding abs for float is a good idea maybe you could obsolete the first one to avoid commit_now
patches accumulating.
Comment 9 Rico Tzschichholz 2016-04-13 14:51:12 UTC
Created attachment 325868 [details] [review]
glib-2.0: Add abs() to float and double
Comment 10 Rico Tzschichholz 2016-04-13 14:53:08 UTC
Review of attachment 325666 [details] [review]:

Replaced by adjusted patch
Comment 11 Rico Tzschichholz 2016-04-13 14:54:11 UTC
(In reply to Al Thomas from comment #8)
> Review of attachment 325800 [details] [review] [review]:
> 
> Thanks for adding abs() for float as well, but both fabs() and fabsf() are
> defined in math.h.
> I know the C function abs() is defined in stdlib.h and maybe that is where
> the confusion comes
> from.

Yes, using math.h here is correct. Updated the patch
Comment 12 Al Thomas 2016-04-13 15:10:28 UTC
Review of attachment 325868 [details] [review]:

Hmm, math.h is already included in the general headers for the binding of gfloat and gdouble.
It's not necessary to have it again for the abs() bindings.
Comment 13 Rico Tzschichholz 2016-04-14 07:05:08 UTC
Attachment 325868 [details] pushed as 5c081e8 - glib-2.0: Add abs() to float and double