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

Jump to bottom

Added GAP_ValueMacFloat to libgap-api #3007

Merged
merged 1 commit into from
Nov 18, 2018
Merged

Added GAP_ValueMacFloat to libgap-api #3007

merged 1 commit into from
Nov 18, 2018

Conversation

sebasguts
Copy link
Member

I decided to use double instead of Double to not having to include macfloats.h into libgap-api.h

fingolfin
fingolfin reviewed Nov 15, 2018
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Hmm, do we also need a wrapper for NEW_MACFLOAT?

fingolfin
fingolfin requested changes Nov 15, 2018
View reviewed changes
src/libgap-api.c Outdated Show resolved Hide resolved
src/libgap-api.h Outdated Show resolved Hide resolved
ChrisJefferson
ChrisJefferson reviewed Nov 15, 2018
View reviewed changes
src/libgap-api.c Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 15, 2018 •
edited
Loading

Codecov Report

Merging #3007 into master will increase coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3007      +/-   ##
==========================================
+ Coverage   85.14%   85.21%   +0.07%     
==========================================
  Files         112      112              
  Lines       56958    57433     +475     
==========================================
+ Hits        48495    48943     +448     
- Misses       8463     8490      +27
Impacted Files Coverage Δ
src/libgap-api.c 20.16% <0%> (-1.4%) ⬇️
src/hpc/threadapi.c 45.19% <0%> (-0.09%) ⬇️
src/error.h 100% <0%> (ø) ⬆️
src/compiled.h 0% <0%> (ø) ⬆️
src/hpc/c_type1.c 86.49% <0%> (ø) ⬆️
src/stats.c 95.27% <0%> (ø) ⬆️
src/hpc/c_oper1.c 92.82% <0%> (+0.01%) ⬆️
src/objset.c 84.71% <0%> (+0.22%) ⬆️
src/listfunc.c 94.4% <0%> (+0.32%) ⬆️
src/calls.c 94.26% <0%> (+0.86%) ⬆️
... and 2 more

@sebasguts
Copy link
Member Author

I hopefully adressed all the comments, and added GAP_NewMacFloat and GAP_IsMacFloat.

fingolfin
fingolfin reviewed Nov 16, 2018
View reviewed changes
src/libgap-api.h Outdated
@@ -197,4 +197,15 @@ extern Int GAP_ValueOfChar(Obj obj);
// Returns the GAP character object with value <obj>.
extern Obj GAP_CharWithValue(UChar obj);

// Assigns the value of the GAP machine float object <obj>
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: There should be a section break comment before this, i.e.,

////
//// floats
////

and ideally this section then should be sorted between the "calls" and "integers" section.

src/libgap-api.c Outdated Show resolved Hide resolved
@sebasguts
Copy link
Member Author

@fingolfin The problem for GAPJulia is even bigger, though.

I tried a lot of things yesterday, and it seems this way you need to create a Float64 Array, pass its data pointer to the function, and then extract its first value. It seems impossible to pass a pointer to a single Float64.

Anyway, what would you suggest instead?

@fingolfin
Copy link
Member

I would suggest to change back to double GAP_ValueMacFloat(Obj obj), as it was before.

@ChrisJefferson
Copy link
Contributor

Sorry to keep changing things.

If it's hard to make a double* in julia, then I wouldn't mind changing back. In general, I would like to make it as hard as possible to break type-safety, but only with interfaces which are actually usable.

@sebasguts
Copy link
Member Author

@ChrisJefferson Do not worry here. I am not completely sure if I am right with the array, it just appeared that way.

@fingolfin Do you suggest removing the typechecks alltogether?

@fingolfin
Copy link
Member

No, there still should be a check calling Error as a fallback, as I suggested from the start.

@sebasguts
Copy link
Member Author

All right then :)

@fingolfin
Copy link
Member

I have no edited the commit in this PR to make it look like I'd prefer: with the floats section delimited by a comment in both .c and .h file; and ordered alphabetically before the integers section; with the check function first, with its comment adjusted to match that of the others; followed by the getter and constructor.

@sebasguts
Copy link
Member Author

@fingolfin okay, thanks a lot :)

@fingolfin fingolfin added topic: kernel release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: libgap things related to libgap labels Nov 18, 2018
@fingolfin fingolfin merged commit 6cd4a7c into gap-system:master Nov 18, 2018
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 15, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson left review comments

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel topic: libgap things related to libgap
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sebasguts @fingolfin @ChrisJefferson @olexandr-konovalov