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.

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

WIP - support for dart26 #57

Merged
merged 11 commits into from
Nov 14, 2019
Merged

WIP - support for dart26 #57

merged 11 commits into from
Nov 14, 2019

Conversation

Buggaboo
Copy link
Contributor

@Buggaboo Buggaboo commented Oct 25, 2019

Relevant changes since 2.6

  • Changed Struct<T> to Struct
  • To dereference a Struct to get to the values you need to ref
  • free and allocate are part of package:ffi as independent functions
  • load and store became a getter/setter called value
  • elementAt has indexing syntax-sugar i.e. ptr[i]
  • asExternalType bla, is replaced with asTypedList to create dart native list types

Update

  • I'm not 100% sure that free(someUtf8Instance) does the job, since Utf8 extends Struct and does not clearly address the Uint8 allocation.
  • To move forward, I think we should reimplement all ByteBuffer related stuff as proper dart Structs.
  • We should check if attachFinalizer is available, and hook on free from there. (TODO anyone else other than me)

At least everything generates, compiles, and the coverage is back to 100%.

This addresses Issue #54

Sources:

We could have used extension methods to fix it without changing too many
lines of code
replace allocate/free with package:ffi's
used asTypedList properly
since there is no clear connection between the alloc of Uint8 and the Utf8 Struct

At least we can generate, compile, run all the tests
@greenrobot greenrobot mentioned this pull request Nov 6, 2019
@nalenz-objectbox
Copy link
Contributor

  • FFI 0.1.3 has been released, so this line can be changed to ffi: ^0.1.3
  • to improve our analytics page, I think it's best to check the arising warnings using pana beforehand, maybe even before pushing
  • unfortunately, it seems like the contribution guide needs to be updated in that case, because pana only accepts code formatted with dartfmt, not dartfmt -l 120
  • Dart 2.6 has been released, so we can use sdk: ">=2.6.0 <3.0.0" as environment dependency. This needs to be changed in the generator as well
  • at least during testing and pana checks, I guess also use objectbox: path: .. instead of the objectbox: 0.4.0 pub.dev dependency for objectbox_generator?
  • the code can be cleaned from a lot of unnecessary imports and other hints and (linter) warnings, as reported by pana
  • in bindings.dart, the following error is reported for lines 157 and 158: The type argument for the pointer, 'NativeFunction<Void Function(Pointer<OBX_bytes_array>)>' must be a 'NativeFunction' in order to use 'asFunction'. Try changing the type argument to be a 'NativeFunction'.

@vaind
Copy link
Contributor

vaind commented Nov 8, 2019

  • unfortunately, it seems like the contribution guide needs to be updated in that case, because pana only accepts code formatted with dartfmt, not dartfmt -l 120

pana takes the -l argument as well. While it will still report on pub.dev, these suggestions have no score impact.

  • at least during testing and pana checks, I guess also use objectbox: path: .. instead of the objectbox: 0.4.0 pub.dev dependency for objectbox_generator?

Hmm, what does that help with?

@nalenz-objectbox
Copy link
Contributor

nalenz-objectbox commented Nov 8, 2019

Hmm, what does that help with?

Actually nothing really, just shortly during development right now, because the current regular objectbox pub.dev package still contains the Dart <=2.5.0 restriction

@vaind vaind changed the base branch from dev-dart26 to dev November 14, 2019 11:17
@vaind vaind merged commit 3d4cd72 into objectbox:dev Nov 14, 2019
@Buggaboo Buggaboo deleted the dev-dart26 branch November 15, 2019 15:59
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