Globals #139

TimWhiting · 2021-01-12T02:42:58Z

@mannprerak2
I think I made the needed changes. Tested it locally and it seems to work.

… keyword

TimWhiting · 2021-01-12T03:17:31Z

Okay, this should be ready for review. The only issue right now is the large integration test, which I'm assuming fails because the library maybe has some globals that weren't generated before?

mannprerak2

Can you also add a new test in test/header_parser_tests? You can pretty much copy the unnamed_enum_test.dart file and modify that to add a few tests. (say for a pointer, int, struct, Long Double, pointer to Long double)

Update: Also update the version in pubspec, changelog, and readme(Add globals in the configuration table in row 6, column 1)

lib/src/header_parser/sub_parsers/var_parser.dart

mannprerak2 · 2021-01-12T06:06:20Z

lib/src/header_parser/sub_parsers/var_parser.dart

+  final type = cursor.type().toCodeGenType();
+  if (type.broadType == BroadType.Unimplemented) {
+    _logger
+        .warning('Global Type not supported $type ${cursor.type().spelling()}');


Add these logs instead.

_logger.fine( '---- Removed Global, reason: unsupported type: ${cursor.completeStringRepr()}'); _logger.warning( "Skipped global variable '$globalName', type not supported.");

mannprerak2 · 2021-01-12T06:17:29Z

lib/src/code_generator/global.dart

    s.write(
-        "${type.getDartType(w)} get $globalVarName => ($holderVarName ??= ${w.dylibIdentifier}.lookup<${type.getCType(w)}>('$originalName')).value;\n\n");
+        "late final ${type.getDartType(w)} $globalVarName = ${w.dylibIdentifier}.lookup<${type.getCType(w)}>('$originalName').$refOrValue;\n\n");


I think global variables are not constants, they can be changed.

If that's the case, we should instead add getters and setters for this.

So for an int global variable, we can do this -

late final ffi.Pointer<ffi.Int32> _globalVar = _dylib.lookup<ffi.Int32>('globalVar'); int get globalVar => _globalVar.value; set globalVar(int value) => _globalVar.value = value;

Yes, this works for all types except structs, which just don't have a setter, but fields for structs can be set from the ref that is obtained via the getter.

lib/src/header_parser/translation_unit_parser.dart

mannprerak2 · 2021-01-12T06:28:42Z

The only issue right now is the large integration test, which I'm assuming fails because the library maybe has some globals that weren't generated before?

The test/debug_generated folder will contain generated output of failed tests, you can compare those with _expected.* files in large_integration_tests folder, and subsequently add the changes there if everything seems okay.

TimWhiting · 2021-01-13T02:28:45Z

@mannprerak2
I addressed everything you mentioned, and added a few features that were missing in my previous implementation (the exclude configuration). Added tests, and updated the _expected file in the large integration test.

mannprerak2

Hi @TimWhiting, Just a few last changes.

lib/src/code_generator/global.dart

lib/src/header_parser/sub_parsers/var_parser.dart

mannprerak2

lgtm! Thanks @TimWhiting

dcharkes

Cool stuff! Some small comments.

lib/src/header_parser/sub_parsers/var_parser.dart

test/header_parser_tests/globals.h

test/header_parser_tests/globals_test.dart

pubspec.yaml

dcharkes · 2021-01-13T16:19:22Z

@TimWhiting

From the CI:

00:13 +40: test/header_parser_tests/globals_test.dart: globals_test (setUpAll)                                                                                                                         
WARNING : Skipped global variable 'longDouble', type not supported.
WARNING : Skipped global variable 'pointerToLongDouble', type not supported.

Are those on purpose?

TimWhiting · 2021-01-13T16:21:53Z

From the CI:

00:13 +40: test/header_parser_tests/globals_test.dart: globals_test (setUpAll)                                                                                                                         
WARNING : Skipped global variable 'longDouble', type not supported.
WARNING : Skipped global variable 'pointerToLongDouble', type not supported.

Are those on purpose?

Yes, that is on purpose. I test that the globals whose types dart:ffi does not support are not in the generated bindings and those messages are logged so the user knows those variables will not be present in the generated bindings.

TimWhiting added 5 commits January 11, 2021 19:39

globals generated

ee5a5f8

remove manual late initialization instead using the non-nullable late…

6dcd229

… keyword

fix naming and globals test

7d41bd6

add typedef dependencies of globals

91651a9

just fixing some naming to make more sense

ff8d605

mannprerak2 suggested changes Jan 12, 2021

View reviewed changes

TimWhiting added 2 commits January 12, 2021 19:21

updates with test, and exclude params

3cf263e

update readme to include globals

ab929f9

mannprerak2 suggested changes Jan 13, 2021

View reviewed changes

lib/src/code_generator/global.dart Outdated Show resolved Hide resolved

lib/src/header_parser/sub_parsers/var_parser.dart Outdated Show resolved Hide resolved

lib/src/header_parser/sub_parsers/var_parser.dart Outdated Show resolved Hide resolved

mannprerak2 reviewed Jan 13, 2021

View reviewed changes

lib/src/header_parser/sub_parsers/var_parser.dart Outdated Show resolved Hide resolved

TimWhiting added 2 commits January 13, 2021 08:52

address review comments

ef5b56f

a last log statement

a50526a

mannprerak2 approved these changes Jan 13, 2021

View reviewed changes

mannprerak2 requested a review from dcharkes January 13, 2021 15:57

dcharkes reviewed Jan 13, 2021

View reviewed changes

address comments

905ba09

dcharkes merged commit 568695c into dart-archive:dart-dev Jan 13, 2021

dcharkes mentioned this pull request Feb 24, 2021

Merge branch 'dart-dev' into 'master'. #154

Closed

mannprerak2 added a commit to mannprerak2/ffigen that referenced this pull request Mar 1, 2021

Added support for Global variables (dart-archive#139)

bf3f7eb

dcharkes pushed a commit that referenced this pull request Mar 1, 2021

Added support for Global variables (#139) (#165)

5f12c73

vaind mentioned this pull request Nov 12, 2020

Use ffigen for C-API bindings objectbox/objectbox-dart#146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Globals #139

Globals #139

TimWhiting commented Jan 12, 2021 •

edited

Loading

TimWhiting commented Jan 12, 2021

mannprerak2 left a comment •

edited

Loading

mannprerak2 Jan 12, 2021

TimWhiting Jan 13, 2021

mannprerak2 Jan 12, 2021

TimWhiting Jan 13, 2021

mannprerak2 commented Jan 12, 2021 •

edited

Loading

TimWhiting commented Jan 13, 2021

mannprerak2 left a comment

mannprerak2 left a comment

dcharkes left a comment

dcharkes commented Jan 13, 2021

TimWhiting commented Jan 13, 2021 •

edited

Loading

Globals #139

Globals #139

Conversation

TimWhiting commented Jan 12, 2021 • edited Loading

TimWhiting commented Jan 12, 2021

mannprerak2 left a comment • edited Loading

Choose a reason for hiding this comment

mannprerak2 Jan 12, 2021

Choose a reason for hiding this comment

TimWhiting Jan 13, 2021

Choose a reason for hiding this comment

mannprerak2 Jan 12, 2021

Choose a reason for hiding this comment

TimWhiting Jan 13, 2021

Choose a reason for hiding this comment

mannprerak2 commented Jan 12, 2021 • edited Loading

TimWhiting commented Jan 13, 2021

mannprerak2 left a comment

Choose a reason for hiding this comment

mannprerak2 left a comment

Choose a reason for hiding this comment

dcharkes left a comment

Choose a reason for hiding this comment

dcharkes commented Jan 13, 2021

TimWhiting commented Jan 13, 2021 • edited Loading

TimWhiting commented Jan 12, 2021 •

edited

Loading

mannprerak2 left a comment •

edited

Loading

mannprerak2 commented Jan 12, 2021 •

edited

Loading

TimWhiting commented Jan 13, 2021 •

edited

Loading