Skip to content

Add support for property queries. #108

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.

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

Merged
merged 20 commits into from
Jul 14, 2020

Conversation

greenrobot-team
Copy link
Member

@greenrobot-team greenrobot-team commented Jun 23, 2020

Squashed and rebased changes from #75

Remaining issues

  • See Help required: query property #75 for remaining issues.
    • .find(replaceNullWith:-1) broken for negative values: it looks like unsigned integers are returned? E.g. 255/65535/4294967295/-1 is returned instead of -1 for 8/16/32/64 bit integers. Wrong types in Structs.
    • Byte unsigned issue is rooted in the binding treating them as unsigned, submitted patch in Unmarshal byte as Int8 (was Uint8). #109.
    • Similar: floats return 0.0 instead of replacement value, but double works fine. Was casting double to float.
  • There are code errors in bindings.dart and signatures.dart.
  • Native property query appears to auto-close/free, e.g. getting results twice crashes, or closing/freeing the property query crashes. Gone, probably wrong pointer types? Odd.
  • Missing from tests: edge cases (e.g. min/max integer, NaN float, etc.).

@greenrobot-team greenrobot-team self-assigned this Jun 23, 2020
@greenrobot-team greenrobot-team force-pushed the Buggaboo-query-property-dev-dart26 branch from 4be15e2 to c2571b6 Compare June 29, 2020 08:59
@greenrobot-team
Copy link
Member Author

greenrobot-team commented Jun 29, 2020

@vaind Two other issues: it looks like for 8-32 bit integers, if their value is negative the property query returns an unsigned value. When doing sums or average they also appear to be treated as unsigned.

Secondly, byte values appear to get returned as unsigned, even via the box interface. Though the property query sum then works correctly. Oh my.

@greenrobot-team greenrobot-team force-pushed the Buggaboo-query-property-dev-dart26 branch from d7a92e0 to 05d7a9e Compare June 29, 2020 12:30
@greenrobot-team
Copy link
Member Author

Resolved integer issue by correcting Structs of int array types to use signed integer pointers.

@greenrobot-team
Copy link
Member Author

Byte unsigned issue is rooted in the binding treating them as unsigned, submitted patch in #109.

This leaves one issue: null floats (not doubles) getting returned as 0.0.

@greenrobot-team
Copy link
Member Author

Was casting double to float, issue resolved as well.

Pending #109 and a final round of clean-up and docs (e.g. on find(), sum() and etc. methods).

@greenrobot-team greenrobot-team force-pushed the Buggaboo-query-property-dev-dart26 branch from a2e674a to ca0650b Compare June 29, 2020 13:36
@greenrobot-team greenrobot-team marked this pull request as ready for review June 30, 2020 07:21
@greenrobot-team greenrobot-team requested a review from vaind June 30, 2020 07:22
@vaind
Copy link
Contributor

vaind commented Jul 13, 2020

@greenrobot-team see the latest commits - should be ready for the merge after #109 is merged so that the tests pass

@vaind
Copy link
Contributor

vaind commented Jul 13, 2020

Just for future reference - the issue with double-free/invalid pointers was caused by a signature mismatch between dart property query function definitions and c-api functions, e.g. obx_query_prop_sum - it was missing the last argument...

@greenrobot-team greenrobot-team force-pushed the Buggaboo-query-property-dev-dart26 branch from d3e34cc to b1766d5 Compare July 13, 2020 13:55
@greenrobot-team
Copy link
Member Author

Rebased onto main with #109 included. Tests all good now. Merging.

@greenrobot-team greenrobot-team merged commit 2ad5e79 into main Jul 14, 2020
@greenrobot-team greenrobot-team deleted the Buggaboo-query-property-dev-dart26 branch July 14, 2020 05:05
@greenrobot-team greenrobot-team mentioned this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants