RFR: Add jextract guide
Add a comprehensive jextract guide under a new `doc/` folder. This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments. Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme. ------------- Commit messages: - Phrasing - Section of pre-precessor defines + other languages - Polish - Update README.md - Create GUIDE.md Changes: https://git.openjdk.org/jextract/pull/231/files Webrev: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=00 Stats: 995 lines in 2 files changed: 824 ins; 171 del; 0 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 3:
1: # Jextract Guide 2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code,
Aren't .c files also viable for jextract? doc/GUIDE.md line 4:
2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code, 4: called 'bindings', which use the Foreign Function and Memory API (FFM API) under the hood,
Double space after `which`. doc/GUIDE.md line 8:
6: 7: Interacting with native C code through the FFM API works 8: by loading a native library (e.g. a `.so`/`.dll`/`.dylib` file), which is essentially an
Comma after `e.g.`. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558058247 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558058784 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558060749
On Tue, 9 Apr 2024 17:29:33 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 3:
1: # Jextract Guide 2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code,
Aren't .c files also viable for jextract?
Technically I don't think the extension matters, but C header files typically have the `.h` extension. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558067083
On Tue, 9 Apr 2024 17:31:47 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 8:
6: 7: Interacting with native C code through the FFM API works 8: by loading a native library (e.g. a `.so`/`.dll`/`.dylib` file), which is essentially an
Comma after `e.g.`.
You mean add a comma like `e.g., ...`? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558233679
On Tue, 9 Apr 2024 20:15:18 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 8:
6: 7: Interacting with native C code through the FFM API works 8: by loading a native library (e.g. a `.so`/`.dll`/`.dylib` file), which is essentially an
Comma after `e.g.`.
You mean add a comma like `e.g., ...`?
Yes. I meant to put these comment in a longer review, but misclicked. Some remarks are more substantial than others. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558267790
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 4:
2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code, 4: called 'bindings', which use the Foreign Function and Memory API (FFM API) under the hood,
maybe add a link for FFM to the final JEP, so that readers who want to know more can do so? doc/GUIDE.md line 4:
2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code, 4: called 'bindings', which use the Foreign Function and Memory API (FFM API) under the hood,
There's several words in the documents which are wrapped in single quotes. Should we use italic here, or bold? doc/GUIDE.md line 15:
13: returned by a lookup, and construct `MemoryLayout` instances for the structs they want to access. 14: Jextract aims to automate many of these steps, so that a client can instead immediately start 15: using a native library they are interested in.
I suggest using `the native library` (because it's the one they are interested in). Also consider using plural (e.g. the native libraries). ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559133910 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559135755 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559138201
On Wed, 10 Apr 2024 09:28:15 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Apply suggestions from code review
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 4:
2: 3: The jextract tool parses header (.h) files of native libraries, and generates Java code, 4: called 'bindings', which use the Foreign Function and Memory API (FFM API) under the hood,
There's several words in the documents which are wrapped in single quotes. Should we use italic here, or bold?
Let's use italic, as that's the style we often use in the javadoc as well
doc/GUIDE.md line 25:
23: ## Running Jextract 24: 25: A native library typically has an `include` directory which contains all the header files
Here I wonder if instead of using pseudo name for paths, we should use some more real-looking names, like `/usr/include` and `/usr/lib`, which are common in many NIX systems.
I kind of wanted to hint at the fact that all library packages should have an `include` directory. The `/usr/include` directory can seem like a special NIX system path where libraries are installed, but that's more a quirk of the package manager which chooses to put the headers of all libraries in the same directory. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559472440 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559465175
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 25:
23: ## Running Jextract 24: 25: A native library typically has an `include` directory which contains all the header files
Here I wonder if instead of using pseudo name for paths, we should use some more real-looking names, like `/usr/include` and `/usr/lib`, which are common in many NIX systems. doc/GUIDE.md line 28:
26: that define the interface of the library, with one 'main' header file. Let's say we have a 27: library called `mylib` stored at `/path/to/mylib` that has a directory `/path/to/mylib/include` 28: with all the header files. And let's say that we have a shell open in the root directory
Maybe instead of `with all the header files` we could expand to `where the header files of that library are stored` doc/GUIDE.md line 29:
27: library called `mylib` stored at `/path/to/mylib` that has a directory `/path/to/mylib/include` 28: with all the header files. And let's say that we have a shell open in the root directory 29: of the Java project we're working on, which has an `src` source directory corresponding to
Suggestion: of the Java project we're working on, which has a `src` source directory corresponding to ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559142532 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559144738 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559141069
On Wed, 10 Apr 2024 09:32:13 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 29:
27: library called `mylib` stored at `/path/to/mylib` that has a directory `/path/to/mylib/include` 28: with all the header files. And let's say that we have a shell open in the root directory 29: of the Java project we're working on, which has an `src` source directory corresponding to
Suggestion:
of the Java project we're working on, which has a `src` source directory corresponding to
It depends on how you read `src`. If you read S R C, then `an` is appropriate.
doc/GUIDE.md line 68:
66: 67: When using the `--library <libspec>` option, the generated code internally uses [`SymbolLookup::libraryLookup`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/forei...)) 68: to load libraries specified by `<libspec>`, after potentially mapping the name of the library to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)).
potentially? Are there cases where mapping a library name is a no-op?
The `<libspec>` might be a path (using `:`), in which case we don't do any mapping, right? I don't mind removing the `potentially` here though.
doc/GUIDE.md line 257:
255: 256: Not all types of macros are supported though. Only macros that have a primitive numerical 257: value, a string, or a pointer type are supported. Most notably, function-like macros are
Several people wonder what to do when dealing with a library that is heavy in its use of function-like macros. There's two strategies here - the first is to replicate the code in the function-like macro in Java, using FFM. The second is to create a small C library which turns the function-like macro into a proper function, so that it can be used from FFM. Not sure we want to add this detail here (or maybe in an appendix).
I think something like that is good to add here. This guide is meant to be comprehensive. If we think some information is useful in the use of jextract, let's add it to the guide. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559449360 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559446730 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559444213
On Wed, 10 Apr 2024 13:33:36 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 68:
66: 67: When using the `--library <libspec>` option, the generated code internally uses [`SymbolLookup::libraryLookup`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/forei...)) 68: to load libraries specified by `<libspec>`, after potentially mapping the name of the library to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)).
potentially? Are there cases where mapping a library name is a no-op?
The `<libspec>` might be a path (using `:`), in which case we don't do any mapping, right? I don't mind removing the `potentially` here though.
ah I see. I mistakenly read `<libspec>` as if it was a library name. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559489798
On Wed, 10 Apr 2024 13:56:11 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
The `<libspec>` might be a path (using `:`), in which case we don't do any mapping, right? I don't mind removing the `potentially` here though.
ah I see. I mistakenly read `<libspec>` as if it was a library name.
In that case, I think it would be clearer if we broke up the sentence. E.g. When using the --library <libspec> option, the generated code internally uses SymbolLookup::libraryLookup to load libraries specified by `<libspec>`. If `<libspec>` denotes a library name, the name is then mapped to a platform dependent name using System::mapLibraryName. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559494614
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 63:
61: Therefore, jextract is intended to be run once, and then for the generated sources to be added to the project. 62: Jextract only needs to be run again when the native library, or jextract itself are updated. (This is also 63: the workflow that jextract itself uses for talking to the libclang library).
Suggestion: the workflow that jextract itself follows: jextract depends on the libclang (link?) native library in order to parse C sources). doc/GUIDE.md line 68:
66: 67: When using the `--library <libspec>` option, the generated code internally uses [`SymbolLookup::libraryLookup`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/forei...)) 68: to load libraries specified by `<libspec>`, after potentially mapping the name of the library to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)).
potentially? Are there cases where mapping a library name is a no-op? doc/GUIDE.md line 72:
70: library loading mechanism on Linux, which is [`dlopen`](https://man7.org/linux/man-pages/man3/dlopen.3.html). 71: This way of loading libraries also relies on OS-specific search mechanisms to find the library file. 72: On Linux the search path can be ammended using the `LD_LIBRARY_PATH` environment variable (see the documentation of `dlopen`).
Suggestion: On Linux the search path can be amended using the `LD_LIBRARY_PATH` environment variable (see the documentation of `dlopen`). ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559150878 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559151912 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559153321
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 135:
133: ### Builtin Type Layouts 134: 135: For every jextract run, regardless of the contents of the library header files, jextract
I'm not sure it's clear where these layouts definitions are introduced. I think the text should say that these are appended to the main header class. doc/GUIDE.md line 253:
251: Note that the enum constants are exposed as top-level methods, rather than being nested 252: inside a class called `MY_ENUM`, or through the use of a Java `enum`. This translation 253: strategy mimics C's behavior of enum constants being accessible as a top-level declaration
There's another reason as well for not using enums - which is that, in C, you can use bitwise operations to denote enum sets - e.g. `RED | GREEN` (and other more crazy stuff), which would not be possible using Java enums. At its core, a C enum is just a glorified int. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559163217 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559168129
On Wed, 10 Apr 2024 09:50:06 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 135:
133: ### Builtin Type Layouts 134: 135: For every jextract run, regardless of the contents of the library header files, jextract
I'm not sure it's clear where these layouts definitions are introduced. I think the text should say that these are appended to the main header class.
Perhaps you can add a comment: // mylib_h.java as you do in later examples? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559164739
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 257:
255: 256: Not all types of macros are supported though. Only macros that have a primitive numerical 257: value, a string, or a pointer type are supported. Most notably, function-like macros are
Several people wonder what to do when dealing with a library that is heavy in its use of function-like macros. There's two strategies here - the first is to replicate the code in the function-like macro in Java, using FFM. The second is to create a small C library which turns the function-like macro into a proper function, so that it can be used from FFM. Not sure we want to add this detail here (or maybe in an appendix). doc/GUIDE.md line 309:
307: ``` 308: 309: There's a getter and setter for each field of the struct (1), which takes a pointer to a struct
I see you didn't use bullet lists much, but I wonder if one would be helpful here instead of a single long para? Perhaps even a numbered list, where the number refers to the comment in the code? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559170731 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559172937
On Wed, 10 Apr 2024 09:58:01 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Apply suggestions from code review
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 309:
307: ``` 308: 309: There's a getter and setter for each field of the struct (1), which takes a pointer to a struct
I see you didn't use bullet lists much, but I wonder if one would be helpful here instead of a single long para? Perhaps even a numbered list, where the number refers to the comment in the code?
I think a numbered list would be appropriate here ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559489693
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 392:
390: ### Function Pointers 391: 392: If jextract finds a function pointer type in the header files it parses, it will generate
More directly: `Jextract will generate a separate class for each function pointer type found in the header files it parses.` doc/GUIDE.md line 420:
418: We again have a meta-data accessor for the function descriptor (`descriptor()`). There's 419: an `allocate` method that can be used to allocate a new instance of this function pointer, 420: who's implementation is implemented by the `fi` functional interface instance. And finally,
Suggestion: whose implementation is defined by the `fi` functional interface instance. And finally, doc/GUIDE.md line 448:
446: 447: Here we use the lambda `(a, b) -> a * b` as the implementation of the `callback_t` instance 448: we create using the call to `allocate`. This method returns an upcall stub like the ones
Suggestion: we create using `allocate`. This method returns an upcall stub like the ones doc/GUIDE.md line 449:
447: Here we use the lambda `(a, b) -> a * b` as the implementation of the `callback_t` instance 448: we create using the call to `allocate`. This method returns an upcall stub like the ones 449: returned by the `java.lang.foreign.Linker::upcallStub` method. The `arena` argument denotes
Should there be a link here - like we did for symbol lookup? doc/GUIDE.md line 451:
449: returned by the `java.lang.foreign.Linker::upcallStub` method. The `arena` argument denotes 450: the lifetime of the upcall stub, meaning that the upcall stub will be freed when the arena 451: is closed (after which the callback instance should no longer be called).
"should not be called" seems to imply that if we call it something bad happens, but we can call it (e.g. it's up to our "goodwill"). "can no longer be called" seems more appropriate? doc/GUIDE.md line 486:
484: `callback_t` instance. 485: 486: Jextract generates function pointer classes like the `callback_t` class for function
I know what you mean here - but I wonder if readers will get it w/o some kind of example. doc/GUIDE.md line 493:
491: 492: Jextract handles variadic functions differently from regular functions. Variadic functions 493: in C more or less like a template, where the calling convention changes based on the number
Suggestion: in C behave more or less like a template, where the calling convention changes based on the number ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559178430 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559179645 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559181255 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559181887 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559183287 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559187645 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559188565
On Wed, 10 Apr 2024 10:06:37 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 451:
449: returned by the `java.lang.foreign.Linker::upcallStub` method. The `arena` argument denotes 450: the lifetime of the upcall stub, meaning that the upcall stub will be freed when the arena 451: is closed (after which the callback instance should no longer be called).
"should not be called" seems to imply that if we call it something bad happens, but we can call it (e.g. it's up to our "goodwill"). "can no longer be called" seems more appropriate?
Of course, if the native code stashes the function pointer in a variable somewhere, it would be possible for it to call the func pointer again. But what I meant is that, in principle, FFM should give you an exception when trying to call `call_me_back` again after the arena has been closed, or calling `callback_t::invoke` on a closed segment. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559185270
On Wed, 10 Apr 2024 10:08:25 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
doc/GUIDE.md line 451:
449: returned by the `java.lang.foreign.Linker::upcallStub` method. The `arena` argument denotes 450: the lifetime of the upcall stub, meaning that the upcall stub will be freed when the arena 451: is closed (after which the callback instance should no longer be called).
"should not be called" seems to imply that if we call it something bad happens, but we can call it (e.g. it's up to our "goodwill"). "can no longer be called" seems more appropriate?
Of course, if the native code stashes the function pointer in a variable somewhere, it would be possible for it to call the func pointer again. But what I meant is that, in principle, FFM should give you an exception when trying to call `call_me_back` again after the arena has been closed, or calling `callback_t::invoke` on a closed segment.
Yes, I had `can no longer be called` before, but switched to `should not be called` since it's technically more correct. I think `can not longer be called` makes more sense for the reader, in hindsight. Thanks ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559499178
On Wed, 10 Apr 2024 10:10:28 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
review comments
doc/GUIDE.md line 486:
484: `callback_t` instance. 485: 486: Jextract generates function pointer classes like the `callback_t` class for function
I know what you mean here - but I wonder if readers will get it w/o some kind of example.
I'll add some examples as well.
doc/GUIDE.md line 665:
663: the elements we specify. 664: 665: To allow for symbol filtering, `jextract` can generate a _dump_ of all the symbols
I noticed that in these sections, refrerences to `jextract` use a code block, whereas in the rest of the document they don't. I don't have a strong preference for one or the other, but we should make that consistent.
I'll just switch to the plain jextract, as that's a little easier on the eyes. (This section was copied from the readme) ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559507298 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559509062
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 14:
12: Java functions using `Linker::upcallStub`, access global variables through the addresses 13: returned by a lookup, and construct `MemoryLayout` instances for the structs they want to access. 14: Jextract aims to automate many of these steps, so that a client can instead immediately start
If you want to avoid capitalization of `jextract` you can say `The jextract tool...` doc/GUIDE.md line 660:
658: ## Filtering 659: 660: Some libraries are incredibly large (such as a platform SDK), and we might not be
Should we just say `windows.h` instead of the more vague "platform SDK" ? doc/GUIDE.md line 665:
663: the elements we specify. 664: 665: To allow for symbol filtering, `jextract` can generate a _dump_ of all the symbols
I noticed that in these sections, refrerences to `jextract` use a code block, whereas in the rest of the document they don't. I don't have a strong preference for one or the other, but we should make that consistent. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559200246 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559196211 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559198874
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 781:
779: 780: Please note that other header files included by jextract may also define macro values using 781: the `#define` pre-processor directive. Therefore it is often important in which order
There seems to be issues with this sentence. Possible suggestion: `It is therefore important to notice the order in which header files are processed by a compiler, as feeding header files to jextract in the wrong order may result in weird errors due to missing macro definitions` doc/GUIDE.md line 808:
806: ### Additional clang options 807: 808: Users can also specify additional clang compiler options, by creating a file named
Maybe here we should start by saying that `jextract` uses clang to parse C files. And if additional options need to be passed to the clang instance used by jextract... doc/GUIDE.md line 814:
812: ## Other Languages 813: 814: As noted in the introduction, jextract currently only supports C header files, but many
Should we say something about many libraries (e.g. libclang) providing a C interop wrapper around their C++ impl? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559204281 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559205783 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559207077
On Wed, 10 Apr 2024 10:26:28 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
doc/GUIDE.md line 814:
812: ## Other Languages 813: 814: As noted in the introduction, jextract currently only supports C header files, but many
Should we say something about many libraries (e.g. libclang) providing a C interop wrapper around their C++ impl?
That's in the table below: `many C++ libraries have a C interface` ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559434905
On Wed, 10 Apr 2024 13:26:10 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 814:
812: ## Other Languages 813: 814: As noted in the introduction, jextract currently only supports C header files, but many
Should we say something about many libraries (e.g. libclang) providing a C interop wrapper around their C++ impl?
That's in the table below: `many C++ libraries have a C interface`
whoops - true. I thought that row talked about manually writing a C wrapper around a C++ API. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559443630
On Wed, 10 Apr 2024 10:25:15 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Split sentence
doc/GUIDE.md line 808:
806: ### Additional clang options 807: 808: Users can also specify additional clang compiler options, by creating a file named
Maybe here we should start by saying that `jextract` uses clang to parse C files. And if additional options need to be passed to the clang instance used by jextract...
I've added a leading sentence here in the latest version: `Jextract uses an embedded clang compiler (through libclang) to parse header files.` ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559525714
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com> ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/01710e76..15776ccc Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=01 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=00-01 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: review comments ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/15776ccc..87be3706 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=02 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=01-02 Stats: 74 lines in 1 file changed: 24 ins; 1 del; 49 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Split sentence ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/87be3706..5bf42921 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=03 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=02-03 Stats: 12 lines in 1 file changed: 3 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Wed, 10 Apr 2024 14:13:37 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Split sentence
Marked as reviewed by mcimadamore (Reviewer). Great job! The guide reads great, and was long overdue :-) doc/GUIDE.md line 407:
405: ### Function Pointers 406: 407: Jextract will generate a separate class for each function pointer type found in the header
Suggestion: Jextract generates a separate class for each function pointer type found in the header doc/GUIDE.md line 505:
503: (such as struct fields or global variables): 504: 505: ```c
thanks! ------------- PR Review: https://git.openjdk.org/jextract/pull/231#pullrequestreview-1991864013 PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2047709012 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559545650 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1559547444
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Phrasing Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com> ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/5bf42921..ab3465b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=04 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Wed, 10 Apr 2024 14:34:25 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
I'm still reviewing, please allow another day before merging. ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2047749223
On Wed, 10 Apr 2024 14:34:25 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
An excellent guide. I read through all of it as someone who already used jextract and left some minor grammar comments that I found to help with the flow, but I would like to point out something more confusing with the structure. The first part is about running jextract, including a short review of an example, followed by an explanation about loading libraries. It looks like this section explains the arguments/options passed to the tool, but it skips the discussion about the macros option (appears much later in Preprocessor Definitions) and other options. The second part is about the code that is generated, but then there is an explanation about `--header-class-name` and `--target-package`, which should be part of the previous part. I would expect that by the time the user gets to learning about the generated Java code, they would know what commands generated it. Also: * Did you leave out "Bitfields" on purpose? * Can a ToC be generated? doc/GUIDE.md line 11:
9: archive of native functions and global variables. The user then has to look up the functions 10: they want to call using a `SymbolLookup`, and finally _link_ the functions by using the 11: `Linker::downcallHandle` method. Additionally, a client may need to create function pointer for
pointer -> pointers doc/GUIDE.md line 18:
16: 17: This guide shows how to run the jextract tool, and how to use the Java code that it generates. 18: The samples under [`samples`](samples) direcotry are also a good source of examples.
"under **the** samples directory" doc/GUIDE.md line 29:
27: library called `mylib` stored at `/path/to/mylib` that has a directory `/path/to/mylib/include` 28: where the header files of that library are stored. And let's say that we have a shell open 29: in the root directory of the Java project we're working on, which has an `src` source
"a `src`" probably. doc/GUIDE.md line 30:
28: where the header files of that library are stored. And let's say that we have a shell open 29: in the root directory of the Java project we're working on, which has an `src` source 30: directory corresponding to,the root package. A typical way to run jextract would be like
Space after the comma. doc/GUIDE.md line 48:
46: - `--include-dir /path/to/mylib/include` specifies a header file search directory, which 47: is used to find header files included through `#include` in the main header file. 48: - `--output src` specifies the root directory for the output. This matches to root package
"matches **the** root" doc/GUIDE.md line 51:
49: of the project's source directory. 50: - `--target-package org.jextract.mylib` specifies the target package to which the generated 51: classes and interfaces will belong. (note that jextract will automatically create the
note -> Note doc/GUIDE.md line 72:
70: to load libraries specified by `<libspec>`. If `<libspec>` denotes a library name, the 71: name is then mapped to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)). 72: This means for instance that on Linux, when specifying `--library mylib` the bindings will
Commas help here: "This means, for instance," doc/GUIDE.md line 75:
73: On Mac the relevant environment variable is `DYLD_LIBRARY_PATH`, and on Windows the variable is `PATH`. 74: Though, for the latter the overall library search mechanism is entirely different (described [here](https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-se...)). 75: When using the HotSpot JVM, the `-Xlog:library` option can als be use to log where the JVM is trying to load a library from,
als -> also use -> used doc/GUIDE.md line 80:
78: and on Windows the variable is `PATH`. Though, for the latter the overall library search 79: mechanism is entirely different (described [here](https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-se...)). 80: When using the HotSpot JVM, the `-Xlog:library` option can als be use to log where the JVM
als -> also use -> used doc/GUIDE.md line 84:
82: 83: The `<libspec>` argument of the `--library` option can either be a library name, such as, 84: `mylib` which will, be mapped to a platform specific name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)), or a path to a library file (either relative or
Comma after `mylib` instead of after `will`. doc/GUIDE.md line 84:
82: 83: The `<libspec>` argument of the `--library` option can either be a library name, such as, 84: `mylib` which will, be mapped to a platform specific name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)), or a path to a library file (either relative or
This was already mentioned in the previous paragraph. doc/GUIDE.md line 85:
83: The `<libspec>` argument of the `--library` option can either be a library name, such as, 84: `mylib` which will, be mapped to a platform specific name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)), or a path to a library file (either relative or 85: absolute) if `<libspec>` is prefixed with the `:` character, such as `mylib.dll`.
Shouldn't the example then be `:mylib.dll`? doc/GUIDE.md line 106:
104: some examples of how to use the generated Java code. 105: 106: Most of the code that jextract generates will be available through a single class. By default
"By default," doc/GUIDE.md line 126:
124: ``` 125: 126: Where `org.mypackage` is the package into which jextract put the generates source files
"puts the generated" doc/GUIDE.md line 134:
132: builtin C types. 133: 134: The latter import statement imports all the other classes the jextract generates, which
"that jextract generates" doc/GUIDE.md line 135:
133: 134: The latter import statement imports all the other classes the jextract generates, which 135: includes: classes representing structs or unions, function types, and struct or union
includes -> include doc/GUIDE.md line 164:
162: `short,` `int`, `long long`, `float`, `double` `long`, and `long double`. Additionally, 163: there is a `C_POINTER` layout which represents the layout for any C pointer type (such as 164: `T*`). Note that these layouts are platform dependent, depending on the platform that
"platform dependent, depending on the platform" This is somewhat redundant. Just "these layouts depend on" maybe, or some other phrasing. doc/GUIDE.md line 167:
165: jextract runs on. For instance, since these constants were generated on Windows, the 166: `long` type has the same layout as the Java `int` type, indicating a 32-bit value, and the 167: `long double` type has the same layout as the Java `double` type. (note that the latter is
note -> Note doc/GUIDE.md line 181:
179: ``` 180: 181: Jextract will generate the following set of methods for this function, in the main header
function, -> function doc/GUIDE.md line 185:
183: 184: ```java 185: // mylib_h.java
Assuming the class name options wasn't specified (if you want to mention that, though not important). doc/GUIDE.md line 197:
195: call the C function (1). Besides that, there are also several accessors that return 196: additional meta-data for the method: the function's address (2), the function descriptor 197: (3), and the method handle returned by the FFM linker (4), which is used to implement the
I would add inline links for convenience, like: `[function's address](MemorySegment)`, `[function descriptor](FunctionDescriptor)` and `(method handle)[MethodHandle]` doc/GUIDE.md line 200:
198: static wrapper method (1). 199: 200: The parameter types and return type of this method depend on the carrier types of the
Is the reader supposed to know what a "carrier type" is? Can a link be given to an explanation if one exists in FFM maybe? doc/GUIDE.md line 211:
209: // mylib.h 210: 211: int bar;
Maybe mention what happens if the definition includes an assignment, `int bar = 4;`, if it matters. doc/GUIDE.md line 269:
267: not supported by jextract. For function-like macros, alternatives include re-writing the 268: code inside the macro in Java, using the FFM API, or writing a small C library which wraps 269: the function-like macro in a proper exported C function, that can then be linked against
"function," -> "function" doc/GUIDE.md line 348:
346: 347: For working with arrays of structs, we can use the `allocateArray` method which accepts an 348: additional element count, indicating the length of the array:
Comma before "which" instead of after "count". doc/GUIDE.md line 358:
356: 357: for (int i = 0; i < arrLen; i++) { 358: MemorySegment element = Point.asSlice(point, i);
Never used this method, but I assume it should be `asSlice(points, i)`. doc/GUIDE.md line 369:
367: In the above example, the `asSlice` method is used to _slice_ out a section of 368: the array, which corresponds to a single `Point` struct element. This method 369: can be used to access individual elements of the `points` array, when given
"array," -> "array" doc/GUIDE.md line 438:
436: we received from native code. 437: 438: For instance, let's say we have a function that accepts an instance of the `callback_t`
For clarity, I would say "have a native function". doc/GUIDE.md line 456:
454: try (Arena arena = Arena.ofConfined()) { 455: Â Â MemorySegment cb = callback_t.allocate((a, b) -> a * b, arena); 456: Â Â int result = call_me_back(cb);
Where is `call_me_back` defined? doc/GUIDE.md line 468:
466: can no longer be called). 467: 468: Additionally, we can using the `callback_t::invoke` method invoke an instance of
using -> use doc/GUIDE.md line 485:
483: 484: The `get_callback` function returns an instance of `callback_t`, which is a function pointer 485: pointing to the native `mult` function. We can call `callback_t` instance that `get_callback()`
"call **the** `callback_t` instance" doc/GUIDE.md line 486:
484: The `get_callback` function returns an instance of `callback_t`, which is a function pointer 485: pointing to the native `mult` function. We can call `callback_t` instance that `get_callback()` 486: return in Java using the `invoke` method in the `callback_t` class that jextract generates
return -> returns doc/GUIDE.md line 534:
532: ``` 533: 534: Jextract doesn't generates a regular method, but a _class_, which represents the invoker:
generates -> generate doc/GUIDE.md line 576:
574: ### Typedefs 575: 576: As mentioned before: typedefs are either translated as a `static final` memory layout fields
before: -> before, doc/GUIDE.md line 603:
601: ``` 602: 603: The `MyPoint` `typedef` on the other hand is a typedef for a struct, so it is translated
"`typedef` on the other hand" -> "`typedef`, on the other hand," doc/GUIDE.md line 618:
616: 617: C allows variable declarations to have an inline anonymous type. Jextract handles in 618: particular cases where a struct's field has an inline type specially. For instance, if we
"Jextract handles in particular cases where a struct's field has an inline type specially." I don't understand this sentence. Does Jextract handle nested types only when a struct's field has an inline type? doc/GUIDE.md line 634:
632: 633: Jextract generates a _nested_ struct and function pointer class for the `bar` and `cb` 634: fields, _inside of_ the class it generates for the `Foo` struct itself:
fields, -> fields doc/GUIDE.md line 713:
711: 712: The include options in this file can then be edited down to a set of symbols that is 713: desired, for instance using other command line tools such as `grep` or `Select-String`,
instance -> instance, doc/GUIDE.md line 800:
798: 799: The value of these macros also affects the behavior of jextract. Therefore, jextract 800: supports setting macro values on the command line using the `-D` or
Perhaps mentioning the distinctions between these macros and the ones described in the Constants section more specifically will be helpful? ------------- PR Review: https://git.openjdk.org/jextract/pull/231#pullrequestreview-1989727538 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562492438 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562494228 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562499511 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562495661 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562498454 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562500829 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562503617 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1558074831 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562505421 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562506125 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562510865 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562511595 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562515695 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562517992 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562519931 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562520377 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562532950 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562534017 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562536552 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562538150 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562544444 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562548168 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562583132 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562620372 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562631106 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562634105 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1562635428 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563001201 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563006011 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563008874 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563038152 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563038500 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563049288 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563054342 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563069603 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563074084 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563075034 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563083141 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563096318
On Fri, 12 Apr 2024 12:49:39 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 84:
82: 83: The `<libspec>` argument of the `--library` option can either be a library name, such as, 84: `mylib` which will, be mapped to a platform specific name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)), or a path to a library file (either relative or
Comma after `mylib` instead of after `will`.
I'd also drop the comma after `such as`
doc/GUIDE.md line 134:
132: builtin C types. 133: 134: The latter import statement imports all the other classes the jextract generates, which
"that jextract generates"
or, `generated by jextract` ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563202919 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563203920
On Fri, 12 Apr 2024 19:39:26 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Did you leave out "Bitfields" on purpose?
Jextract does not support bitfields (due to deficiencies in the libclang API). That's why there's no section for them :-)
doc/GUIDE.md line 29:
27: library called `mylib` stored at `/path/to/mylib` that has a directory `/path/to/mylib/include` 28: where the header files of that library are stored. And let's say that we have a shell open 29: in the root directory of the Java project we're working on, which has an `src` source
"a `src`" probably.
I believe this was discussed here: https://github.com/openjdk/jextract/pull/231#discussion_r1559141069
doc/GUIDE.md line 348:
346: 347: For working with arrays of structs, we can use the `allocateArray` method which accepts an 348: additional element count, indicating the length of the array:
Comma before "which" instead of after "count".
I find the way it's currently written fine (and better than the suggested one) ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2052541652 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563207432 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1563205742
On Fri, 12 Apr 2024 21:15:08 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Did you leave out "Bitfields" on purpose?
Jextract does not support bitfields (due to deficiencies in the libclang API). That's why there's no section for them :-)
Then maybe mention that :) ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2053511126
On Sat, 13 Apr 2024 05:36:09 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Did you leave out "Bitfields" on purpose?
Jextract does not support bitfields (due to deficiencies in the libclang API). That's why there's no section for them :-)
Then maybe mention that :)
I've added a section about unsupported features. (also to explain some of the warnings a user may see) ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2056835517
On Fri, 12 Apr 2024 19:39:26 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
The first part is about running jextract, including a short review of an example, followed by an explanation about loading libraries. It looks like this section explains the arguments/options passed to the tool, but it skips the discussion about the macros option (appears much later in Preprocessor Definitions) and other options.
The second part is about the code that is generated, but then there is an explanation about `--header-class-name` and `--target-package`, which should be part of the previous part. I would expect that by the time the user gets to learning about the generated Java code, they would know what commands generated it.
Yes. The intent of this was to not bog down the intro with information that is not used that often, so people can get started quickly. But, in hindsight, I think it is better to just put all the discussion about command line arguments in 1 place. (Especially some of the info in the section about `-D` is important to get jextract to 'work'). I'll re-arrange the sections a bit. ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2056751066
On Fri, 12 Apr 2024 13:49:21 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 211:
209: // mylib.h 210: 211: int bar;
Maybe mention what happens if the definition includes an assignment, `int bar = 4;`, if it matters.
For jextract it doesn't matter
doc/GUIDE.md line 358:
356: 357: for (int i = 0; i < arrLen; i++) { 358: MemorySegment element = Point.asSlice(point, i);
Never used this method, but I assume it should be `asSlice(points, i)`.
Good catch!
doc/GUIDE.md line 456:
454: try (Arena arena = Arena.ofConfined()) { 455: Â Â MemorySegment cb = callback_t.allocate((a, b) -> a * b, arena); 456: Â Â int result = call_me_back(cb);
Where is `call_me_back` defined?
It's defined in the C file in the snippet above.
doc/GUIDE.md line 618:
616: 617: C allows variable declarations to have an inline anonymous type. Jextract handles in 618: particular cases where a struct's field has an inline type specially. For instance, if we
"Jextract handles in particular cases where a struct's field has an inline type specially."
I don't understand this sentence. Does Jextract handle nested types only when a struct's field has an inline type?
C doesn't allow other kinds of nested types, so yeah ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565748513 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565750938 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565752183 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565755721
On Mon, 15 Apr 2024 12:59:53 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 618:
616: 617: C allows variable declarations to have an inline anonymous type. Jextract handles in 618: particular cases where a struct's field has an inline type specially. For instance, if we
"Jextract handles in particular cases where a struct's field has an inline type specially."
I don't understand this sentence. Does Jextract handle nested types only when a struct's field has an inline type?
C doesn't allow other kinds of nested types, so yeah
I'll just remove the 'Jextract handles in particular cases where a struct's field has an inline type specially.' part here ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565757187
On Mon, 15 Apr 2024 12:57:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 456:
454: try (Arena arena = Arena.ofConfined()) { 455: Â Â MemorySegment cb = callback_t.allocate((a, b) -> a * b, arena); 456: Â Â int result = call_me_back(cb);
Where is `call_me_back` defined?
It's defined in the C file in the snippet above.
But we can't call the c function directly. Are there more java bindings that jextract generates? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565766202
On Mon, 15 Apr 2024 13:07:34 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
It's defined in the C file in the snippet above.
But we can't call the c function directly. Are there more java bindings that jextract generates?
Sorry, the example assumes that that the `call_me_back` function is also defined in the header file that jextract extracted, so a Java method would be generated for it. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565784414
On Mon, 15 Apr 2024 13:19:45 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
But we can't call the c function directly. Are there more java bindings that jextract generates?
Sorry, the example assumes that that the `call_me_back` function is also defined in the header file that jextract extracted, so a Java method would be generated for it.
I would mentioned that. After all, this tool is meant to save Java developers from knowing the intricacies of the native language. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565817933
On Mon, 15 Apr 2024 13:39:47 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Sorry, the example assumes that that the `call_me_back` function is also defined in the header file that jextract extracted, so a Java method would be generated for it.
I would mentioned that. After all, this tool is meant to save Java developers from knowing the intricacies of the native language.
Was this resolved? I don't see a clarification (maybe I'm missing it). ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566554742
On Mon, 15 Apr 2024 23:51:35 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
I would mentioned that. After all, this tool is meant to save Java developers from knowing the intricacies of the native language.
Was this resolved? I don't see a clarification (maybe I'm missing it).
I put this: We can call this function from Java as follows (assuming this function is also exported through the header files passed to jextract) ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566596144
On Fri, 12 Apr 2024 19:38:20 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 800:
798: 799: The value of these macros also affects the behavior of jextract. Therefore, jextract 800: supports setting macro values on the command line using the `-D` or
Perhaps mentioning the distinctions between these macros and the ones described in the Constants section more specifically will be helpful?
Hmm, not sure what to say. There are no real distinctions, but jextract only generates an accessor if a macro is defined in the header files, not if it is only set on the command line with `-D`. (I'll add that at least) ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565763762
On Fri, 12 Apr 2024 19:39:26 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Can a ToC be generated?
It should be generated automatically by GitHub ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2056814832
On Fri, 12 Apr 2024 13:16:16 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 185:
183: 184: ```java 185: // mylib_h.java
Assuming the class name options wasn't specified (if you want to mention that, though not important).
Don't think it's that important to mention. The comment is mostly there for consistency, as the text above already says that this is the class that jextract generates.
doc/GUIDE.md line 200:
198: static wrapper method (1). 199: 200: The parameter types and return type of this method depend on the carrier types of the
Is the reader supposed to know what a "carrier type" is? Can a link be given to an explanation if one exists in FFM maybe?
'carrier type` is indeed an FFM concept. I've added a link to `FunctionDescriptor::toMethodType` which explains how these are derived. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565790719 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565789199
On Wed, 10 Apr 2024 14:34:25 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Phrasing
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 757:
755: 756: To enable the tracing support, just pass the `-Djextract.trace.downcalls=true` flag to the 757: launcher used to start the application. Below we show an excerpt of the output when
I would mention that this is passed as a VM argument, not as a program argument. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1564920967
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: review comments ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/ab3465b2..b9218138 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=05 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=04-05 Stats: 159 lines in 1 file changed: 73 ins; 41 del; 45 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Mon, 15 Apr 2024 13:25:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
review comments
doc/GUIDE.md line 79:
77: [libclang](https://clang.llvm.org/docs/LibClang.html) native library in order to parse C sources). 78: 79: ### Preprocessor Definitions
IMHO both this and the following section is not important enough to appear so early in the document. If we went straight to "Using The Code Generated By Jextract" we would not lose anything - then after that section, we can provide more details about various aspects/options. doc/GUIDE.md line 147:
145: some examples of how to use the generated Java code. 146: 147: Most of the methods that jextract generates are `static`, and are designed to be imported
Uhm - I think the paras you moved fitted nicely in this introductory section doc/GUIDE.md line 226:
224: First and foremost, there is a static wrapper method that is generated that can be used to 225: call the C function (1). Besides that, there are also several accessors that return 226: additional meta-data for the method: the function's address, represented as a
Should we use a numbered list here too? doc/GUIDE.md line 306:
304: 305: Note that for macros, jextract only generates an accessor when it sees a macro definition, 306: like the one in the example, in the header files it parses. When a macro is only defined
Suggestion: like the one in the example, in the header files it parses. When a macro is defined doc/GUIDE.md line 307:
305: Note that for macros, jextract only generates an accessor when it sees a macro definition, 306: like the one in the example, in the header files it parses. When a macro is only defined 307: using `-D` on the command line, not accessor will be generated.
Suggestion: using `-D` on the command line, no accessor will be generated. doc/GUIDE.md line 848:
846: ## Unsupported Features 847: 848: There are several elements for which jextract can not generate bindings:
element seems a bit vague, maybe "C program elements" could be better, although in this cases I suppose most developers will think about the term "features". Consider just having a bullet list and skip the first statement (after all, the title says it all). Note also the list is provided here: https://jdk.java.net/jextract/ (maybe we should drop it from there in the future, to avoid duplication?) doc/GUIDE.md line 869:
867: WARNING: Skipping Foo (type Declared(Foo) is not supported) 868: ``` 869:
Bitfields missing? Also, support for primitive types > 64 bits. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565821418 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565806681 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565809259 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565810496 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565810263 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565815548 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565816332
On Mon, 15 Apr 2024 13:32:21 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
review comments
doc/GUIDE.md line 147:
145: some examples of how to use the generated Java code. 146: 147: Most of the methods that jextract generates are `static`, and are designed to be imported
Uhm - I think the paras you moved fitted nicely in this introductory section
I think they could go in either section? But, this way, we can discuss the `--header-class-name` option in the section about running, which I think makes more sense.
doc/GUIDE.md line 226:
224: First and foremost, there is a static wrapper method that is generated that can be used to 225: call the C function (1). Besides that, there are also several accessors that return 226: additional meta-data for the method: the function's address, represented as a
Should we use a numbered list here too?
I don't think it's possible to start a markdown list at `2`, but I will try
doc/GUIDE.md line 306:
304: 305: Note that for macros, jextract only generates an accessor when it sees a macro definition, 306: like the one in the example, in the header files it parses. When a macro is only defined
Suggestion:
like the one in the example, in the header files it parses. When a macro is defined
`only` is relevant here. If a macro is defined using `#define` _and_ on the command line, jextract will generate an accessor.
doc/GUIDE.md line 869:
867: WARNING: Skipping Foo (type Declared(Foo) is not supported) 868: ``` 869:
Bitfields missing? Also, support for primitive types > 64 bits.
Bitfields is there. I'll add an item for types > 64 bits ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565813726 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565814932 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565816756 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565820708
On Mon, 15 Apr 2024 13:41:29 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 869:
867: WARNING: Skipping Foo (type Declared(Foo) is not supported) 868: ``` 869:
Bitfields missing? Also, support for primitive types > 64 bits.
Bitfields is there. I'll add an item for types > 64 bits
I see bitfields now, but we should say something for e.g. `long double` on Linux ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565824125
On Mon, 15 Apr 2024 13:37:01 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 147:
145: some examples of how to use the generated Java code. 146: 147: Most of the methods that jextract generates are `static`, and are designed to be imported
Uhm - I think the paras you moved fitted nicely in this introductory section
I think they could go in either section? But, this way, we can discuss the `--header-class-name` option in the section about running, which I think makes more sense.
On a second look, it makes sense in this updated version. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565848813
On Mon, 15 Apr 2024 13:37:51 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 226:
224: First and foremost, there is a static wrapper method that is generated that can be used to 225: call the C function (1). Besides that, there are also several accessors that return 226: additional meta-data for the method: the function's address, represented as a
Should we use a numbered list here too?
I don't think it's possible to start a markdown list at `2`, but I will try
Looks like this doesn't work unfortunately. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565850823
On Mon, 15 Apr 2024 14:00:46 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
I don't think it's possible to start a markdown list at `2`, but I will try
Looks like this doesn't work unfortunately.
No, wait, got it working after all. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565852284
On Mon, 15 Apr 2024 13:39:03 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 306:
304: 305: Note that for macros, jextract only generates an accessor when it sees a macro definition, 306: like the one in the example, in the header files it parses. When a macro is only defined
Suggestion:
like the one in the example, in the header files it parses. When a macro is defined
`only` is relevant here. If a macro is defined using `#define` _and_ on the command line, jextract will generate an accessor.
I understand what you wanted to stress - but IMHO `only` is still redundant. E.g. "when a macro is defined using -D" is still plenty clear? Using `only` seems to convey some kind of deeper distinction e.g. that macros with -D are somehow "less macros" than the other ones. At least it's how it reads to me. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565866984
On Mon, 15 Apr 2024 14:11:13 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
`only` is relevant here. If a macro is defined using `#define` _and_ on the command line, jextract will generate an accessor.
I understand what you wanted to stress - but IMHO `only` is still redundant. E.g. "when a macro is defined using -D" is still plenty clear? Using `only` seems to convey some kind of deeper distinction e.g. that macros with -D are somehow "less macros" than the other ones. At least it's how it reads to me.
Ok, removed. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566521615
On Mon, 15 Apr 2024 13:41:51 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
review comments
doc/GUIDE.md line 79:
77: [libclang](https://clang.llvm.org/docs/LibClang.html) native library in order to parse C sources). 78: 79: ### Preprocessor Definitions
IMHO both this and the following section is not important enough to appear so early in the document. If we went straight to "Using The Code Generated By Jextract" we would not lose anything - then after that section, we can provide more details about various aspects/options.
Not sure... I know what you mean, but knowing how to run the generated code seems fairly important. It seems that that is something someone might want to try out after the first example. Unfortunately, this requires understanding library loading, which is fairly complex. Also, the comment about feeding jextract the right header file seems pretty important since user may run into errors when running jextract otherwise (we've seen many questions about that, e.g. when someone tries to extract some arbitrary Windows header instead of `Windows.h`). But also, I realized that this guide may not just be used by first-time readers, but also as a reference by more experienced users, in which case it seems nice to have info about command line flags mostly in one place.
doc/GUIDE.md line 848:
846: ## Unsupported Features 847: 848: There are several elements for which jextract can not generate bindings:
element seems a bit vague, maybe "C program elements" could be better, although in this cases I suppose most developers will think about the term "features". Consider just having a bullet list and skip the first statement (after all, the title says it all). Note also the list is provided here: https://jdk.java.net/jextract/ (maybe we should drop it from there in the future, to avoid duplication?)
Yeah, we probably just want to link to the guide from the download page ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565837436 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565838811
On Mon, 15 Apr 2024 13:52:07 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 79:
77: [libclang](https://clang.llvm.org/docs/LibClang.html) native library in order to parse C sources). 78: 79: ### Preprocessor Definitions
IMHO both this and the following section is not important enough to appear so early in the document. If we went straight to "Using The Code Generated By Jextract" we would not lose anything - then after that section, we can provide more details about various aspects/options.
Not sure... I know what you mean, but knowing how to run the generated code seems fairly important. It seems that that is something someone might want to try out after the first example. Unfortunately, this requires understanding library loading, which is fairly complex.
Also, the comment about feeding jextract the right header file seems pretty important since user may run into errors when running jextract otherwise (we've seen many questions about that, e.g. when someone tries to extract some arbitrary Windows header instead of `Windows.h`).
But also, I realized that this guide may not just be used by first-time readers, but also as a reference by more experienced users, in which case it seems nice to have info about command line flags mostly in one place.
While it's true that knowing which header to extract is important, I think having a whole section on "preprocessor definitions" feels like having a very cornery section in a place where in reality you want something more general. E.g. extracting windows.h is tricky, because of order dependencies, but I'm not sure the user needs to be aware of -D here. Just mentioning that headers are complex beasts and can depend on each other in unpredictable ways (and refer to the doc of the library that needs to be extracted) should be good enough guidance at this point. As for library loading I'm not sure... again, yes, we probably need to say something like "you need to ensure the library you want to call is loaded first" - but I'm not sure we need the full shebang of library loading options. In other places you added references to stuff that is further expanded upon in later sections - e.g. Besides these options, it is also possible to filter the output of jextract using one of the --include-XXX options that jextract has. See the section on [filtering](https://github.com/JornVernee/jextract/blob/Guide/doc/GUIDE.md#filtering) for a more detailed overview. Seems like something similar is required here. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565856181
On Mon, 15 Apr 2024 14:04:16 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Not sure... I know what you mean, but knowing how to run the generated code seems fairly important. It seems that that is something someone might want to try out after the first example. Unfortunately, this requires understanding library loading, which is fairly complex.
Also, the comment about feeding jextract the right header file seems pretty important since user may run into errors when running jextract otherwise (we've seen many questions about that, e.g. when someone tries to extract some arbitrary Windows header instead of `Windows.h`).
But also, I realized that this guide may not just be used by first-time readers, but also as a reference by more experienced users, in which case it seems nice to have info about command line flags mostly in one place.
While it's true that knowing which header to extract is important, I think having a whole section on "preprocessor definitions" feels like having a very cornery section in a place where in reality you want something more general. E.g. extracting windows.h is tricky, because of order dependencies, but I'm not sure the user needs to be aware of -D here. Just mentioning that headers are complex beasts and can depend on each other in unpredictable ways (and refer to the doc of the library that needs to be extracted) should be good enough guidance at this point.
As for library loading I'm not sure... again, yes, we probably need to say something like "you need to ensure the library you want to call is loaded first" - but I'm not sure we need the full shebang of library loading options. In other places you added references to stuff that is further expanded upon in later sections - e.g.
Besides these options, it is also possible to filter the output of jextract using one of the --include-XXX options that jextract has. See the section on [filtering](https://github.com/JornVernee/jextract/blob/Guide/doc/GUIDE.md#filtering) for a more detailed overview.
Seems like something similar is required here.
Still not sure. The section on pre-processor definitions is already fairly small, and in some rare cases, `-D` may be required for extraction to work as well. Do we really want to put ~half this section elsewhere? It seems this information is relevant to 'Running Jextract' as well. For library loading, a user needs to know what happens to the string they pass to `--library`, which requires understanding the distinction between library names/paths, and the fact that the user needs to set `LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH`/`PATH`. Even for someone who's experienced with native library loading, that seems like important information? I think in both cases, we would end up splitting just part of the information into a separate section. Even if not ideal, I think it's better to keep all the info together. So, if we can't get away with moving these sections out-of-line completely, which I don't think we can, I'm inclined to leave things as they are in this area. Maybe we can improve the situation by having a very basic hello world example up front? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565935848
On Mon, 15 Apr 2024 14:55:44 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
While it's true that knowing which header to extract is important, I think having a whole section on "preprocessor definitions" feels like having a very cornery section in a place where in reality you want something more general. E.g. extracting windows.h is tricky, because of order dependencies, but I'm not sure the user needs to be aware of -D here. Just mentioning that headers are complex beasts and can depend on each other in unpredictable ways (and refer to the doc of the library that needs to be extracted) should be good enough guidance at this point.
As for library loading I'm not sure... again, yes, we probably need to say something like "you need to ensure the library you want to call is loaded first" - but I'm not sure we need the full shebang of library loading options. In other places you added references to stuff that is further expanded upon in later sections - e.g.
Besides these options, it is also possible to filter the output of jextract using one of the --include-XXX options that jextract has. See the section on [filtering](https://github.com/JornVernee/jextract/blob/Guide/doc/GUIDE.md#filtering) for a more detailed overview.
Seems like something similar is required here.
Still not sure. The section on pre-processor definitions is already fairly small, and in some rare cases, `-D` may be required for extraction to work as well. Do we really want to put ~half this section elsewhere? It seems this information is relevant to 'Running Jextract' as well.
For library loading, a user needs to know what happens to the string they pass to `--library`, which requires understanding the distinction between library names/paths, and the fact that the user needs to set `LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH`/`PATH`. Even for someone who's experienced with native library loading, that seems like important information? (i.e. because it's not clear how jextract handles this)
I think in both cases, we would end up splitting just part of the information into a separate section. Even if not ideal, I think it's better to keep all the info together. So, if we can't get away with moving these sections out-of-line completely, which I don't think we can, I'm inclined to leave things as they are in this area.
Maybe we can improve the situation by having a very basic hello world example up front?
IMHO is not about moving "half sections", but about writing a section that is correct for the purpose and context in which it appears in the document. We seem to have two requirements for users getting started with jextract: * explain that the header on which it is run matters (this can literally be one sentence) * explain that libraries must be loaded (I think here we can get away by providing something that works in the 80% use cases, plus some comment on LD_LIBRARY_PATH, and refer everything else to a more specialized section. E.g. I don't think here we should be talking about different lookups, in which orders symbols are looked up etc.). This way you can have a document that serves both "green" developers and more experienced one (the latter will want to keep going after the section illustrating how to use bindings). At least, IMHO. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565951070
On Mon, 15 Apr 2024 15:05:22 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Still not sure. The section on pre-processor definitions is already fairly small, and in some rare cases, `-D` may be required for extraction to work as well. Do we really want to put ~half this section elsewhere? It seems this information is relevant to 'Running Jextract' as well.
For library loading, a user needs to know what happens to the string they pass to `--library`, which requires understanding the distinction between library names/paths, and the fact that the user needs to set `LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH`/`PATH`. Even for someone who's experienced with native library loading, that seems like important information? (i.e. because it's not clear how jextract handles this)
I think in both cases, we would end up splitting just part of the information into a separate section. Even if not ideal, I think it's better to keep all the info together. So, if we can't get away with moving these sections out-of-line completely, which I don't think we can, I'm inclined to leave things as they are in this area.
Maybe we can improve the situation by having a very basic hello world example up front?
IMHO is not about moving "half sections", but about writing a section that is correct for the purpose and context in which it appears in the document. We seem to have two requirements for users getting started with jextract: * explain that the header on which it is run matters (this can literally be one sentence) * explain that libraries must be loaded (I think here we can get away by providing something that works in the 80% use cases, plus some comment on LD_LIBRARY_PATH, and refer everything else to a more specialized section. E.g. I don't think here we should be talking about different lookups, in which orders symbols are looked up etc.).
This way you can have a document that serves both "green" developers and more experienced one (the latter will want to keep going after the section illustrating how to use bindings). At least, IMHO.
I gave this a try in the latest version. Still not sure which one is better, but I don't mind either way. I put all the sections after the examples in an 'Advanced' section as well. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565981693
On Mon, 15 Apr 2024 15:26:59 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
IMHO is not about moving "half sections", but about writing a section that is correct for the purpose and context in which it appears in the document. We seem to have two requirements for users getting started with jextract: * explain that the header on which it is run matters (this can literally be one sentence) * explain that libraries must be loaded (I think here we can get away by providing something that works in the 80% use cases, plus some comment on LD_LIBRARY_PATH, and refer everything else to a more specialized section. E.g. I don't think here we should be talking about different lookups, in which orders symbols are looked up etc.).
This way you can have a document that serves both "green" developers and more experienced one (the latter will want to keep going after the section illustrating how to use bindings). At least, IMHO.
I gave this a try in the latest version. Still not sure which one is better, but I don't mind either way. I put all the sections after the examples in an 'Advanced' section as well.
I personally like the new changes a lot! I find the "running jextract" section very informative, providing all the major info w/o overstaying its welcome. Also it points to several other sections in the doc, so it acts a bit as a mental map, which is good. I have some reservation about "unsupported features" being down in "Advanced" - should that be the last section in "Using jextract bindings" instead? (e.g. first we show what works, and then if the reader was expecting something else, we have a section for that too). Or, maybe pull it more "up front". Right now it seems kind of buried in other stuff, even thought it seems generally useful. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565996431
On Mon, 15 Apr 2024 15:37:57 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
I gave this a try in the latest version. Still not sure which one is better, but I don't mind either way. I put all the sections after the examples in an 'Advanced' section as well.
I personally like the new changes a lot! I find the "running jextract" section very informative, providing all the major info w/o overstaying its welcome. Also it points to several other sections in the doc, so it acts a bit as a mental map, which is good.
I have some reservation about "unsupported features" being down in "Advanced" - should that be the last section in "Using jextract bindings" instead? (e.g. first we show what works, and then if the reader was expecting something else, we have a section for that too). Or, maybe pull it more "up front". Right now it seems kind of buried in other stuff, even thought it seems generally useful.
Ok, I'll keep it like this then. I'll move the unsupported feature to the end of the 'using' section. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566019600
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - Update doc/GUIDE.md Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com> - add note about call_me_back needing to be present in header file as well ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/b9218138..a9e7bb8b Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=06 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=05-06 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - try fix numbered list - more review comments + add para about default library loading ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/a9e7bb8b..af44d7fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=07 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=06-07 Stats: 16 lines in 1 file changed: 9 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Mon, 15 Apr 2024 14:04:24 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision:
- try fix numbered list - more review comments + add para about default library loading
doc/GUIDE.md line 234:
232: call the C function (1). Besides that, there are also several accessors that return 233: additional meta-data for the method: 234:
You could also add an item for (1) instead of having separate text above? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565861749
On Mon, 15 Apr 2024 14:07:54 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision:
- try fix numbered list - more review comments + add para about default library loading
doc/GUIDE.md line 234:
232: call the C function (1). Besides that, there are also several accessors that return 233: additional meta-data for the method: 234:
You could also add an item for (1) instead of having separate text above?
Done ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565865457
On Mon, 15 Apr 2024 14:10:16 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 234:
232: call the C function (1). Besides that, there are also several accessors that return 233: additional meta-data for the method: 234:
You could also add an item for (1) instead of having separate text above?
Done
Now veering into nitpicks: but I note that we have an asymmetry between structs and functions. In functions we try to call out the function accessor (probably on the basis that this is the symbol the user is likely to be looking for). But for structs we just list all the members one after the other (even though the most likely is probably (1) - e.g. getter/setter). I think the document could be improved (optionally) by rectifying this one way or the other. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1565871964
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: more list items ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/af44d7fc..a68da7b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=08 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=07-08 Stats: 7 lines in 1 file changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: move defines & library loading sections out-of-line ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/a68da7b4..fd4d067c Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=09 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=08-09 Stats: 159 lines in 1 file changed: 84 ins; 69 del; 6 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: move unsupported features to end of using section ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/fd4d067c..4491aa6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=10 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=09-10 Stats: 48 lines in 1 file changed: 25 ins; 23 del; 0 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Mon, 15 Apr 2024 15:58:16 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
move unsupported features to end of using section
I think I've addressed all review comments. Please have another look ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2057947555
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Update doc/GUIDE.md Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com> ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/4491aa6f..5de1f4fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=11 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Mon, 15 Apr 2024 23:01:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
Looks very good. Left some minor comments. Can be integrated after they are resolved. README.md line 5:
3: `jextract` is a tool which mechanically generates Java bindings from a native library headers. This tools leverages the [clang C API](https://clang.llvm.org/doxygen/group__CINDEX.html) in order to parse the headers associated with a given native library, and the generated Java bindings build upon the [Foreign Function & Memory API](https://openjdk.java.net/jeps/454). The `jextract` tool was originally developed in the context of [Project Panama](https://openjdk.java.net/projects/panama/) (and then made available in the Project Panama [Early Access binaries](https://jdk.java.net/panama/)). 4: 5: :bulb: For instruction on how to use the jextract tool, please refer to the guide [here](doc/GUIDE.md)
Period at the end. doc/GUIDE.md line 55:
53: through `--output`) 54: - `--library mylib` tells jextract that the generated bindings should load the library 55: called `mylib`. (The section on [library loading](#library-loading) discusses how is done)
"how is done" -> "how it's done." doc/GUIDE.md line 59:
57: Note that specifying the wrong header file to jextract may result in errors during parsing. 58: Please consult the documentation of the library in question about which header file 59: should be included. This is also the header files that should be passed to jextract. If
These sentences mix plural and singular header file(s). doc/GUIDE.md line 266:
264: using `-D` on the command line, no accessor will be generated. 265: 266: ### Structs & Unions
This section follows an example using a struct. If there's no difference whatsoever compared to a union, this is fine. Otherwise, I will mention in the example where things might differ. doc/GUIDE.md line 312:
310: Â Â public static MemorySegment reinterpret(MemorySegment addr, long elementCount, 311: Arena arena, Consumer<MemorySegment> cleanup) { ... } // 6 312: }
I noticed that some of these methods are `final` and some aren't. Is this on purpose in jextract (why?), or just here? doc/GUIDE.md line 739:
737: name is then mapped to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)). 738: This means, for instance, that on Linux, when specifying `--library mylib`, the bindings will 739: try to load `libmylib.so` using the OS-specific library loading mechanism on Linux, which
`libmylib.so` -> not `mylib.so`? doc/GUIDE.md line 896:
894: 895: Jextract uses an embedded clang compiler (through libclang) to parse header files. Users 896: can also specify additional clang compiler options, by creating a file named
"options," -> "options" ------------- PR Review: https://git.openjdk.org/jextract/pull/231#pullrequestreview-2002311013 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566522599 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566523022 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566525138 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566544393 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566542910 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566560650 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566562931
On Mon, 15 Apr 2024 23:36:30 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 266:
264: using `-D` on the command line, no accessor will be generated. 265: 266: ### Structs & Unions
This section follows an example using a struct. If there's no difference whatsoever compared to a union, this is fine. Otherwise, I will mention in the example where things might differ.
It's exactly the same for both (I'll mention that as well)
doc/GUIDE.md line 312:
310: Â Â public static MemorySegment reinterpret(MemorySegment addr, long elementCount, 311: Arena arena, Consumer<MemorySegment> cleanup) { ... } // 6 312: }
I noticed that some of these methods are `final` and some aren't. Is this on purpose in jextract (why?), or just here?
static methods don't need to be `final`. I think that may be an artifact of some code sharing we do internally.
doc/GUIDE.md line 739:
737: name is then mapped to a platform dependent name using [`System::mapLibraryName`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Syste...)). 738: This means, for instance, that on Linux, when specifying `--library mylib`, the bindings will 739: try to load `libmylib.so` using the OS-specific library loading mechanism on Linux, which
`libmylib.so` -> not `mylib.so`?
No, `mapLibraryName` adds the `lib` prefix on Linux ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566593004 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566592704 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566593792
On Mon, 15 Apr 2024 23:01:03 GMT, Nir Lisker <nlisker@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 55:
53: through `--output`) 54: - `--library mylib` tells jextract that the generated bindings should load the library 55: called `mylib`. (The section on [library loading](#library-loading) discusses how is done)
"how is done" -> "how it's done."
This should be have "this is" ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566596703
On Mon, 15 Apr 2024 23:01:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 10:
8: by loading a native library (e.g., a `.so`/`.dll`/`.dylib` file), which is essentially an 9: archive of native functions and global variables. The user then has to look up the functions 10: they want to call using a `SymbolLookup`, and finally _link_ the functions by using the
There's some classes and methods referenced here - should we use links? (don't have strong opinion) doc/GUIDE.md line 20:
18: The samples under the [`samples`](samples) direcotry are also a good source of examples. 19: 20: Note that at this time, jextract only supports C header files. If you have a library written
should we say "jextract (and FFM) only support..." ? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566951897 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566953749
On Tue, 16 Apr 2024 08:27:17 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 10:
8: by loading a native library (e.g., a `.so`/`.dll`/`.dylib` file), which is essentially an 9: archive of native functions and global variables. The user then has to look up the functions 10: they want to call using a `SymbolLookup`, and finally _link_ the functions by using the
There's some classes and methods referenced here - should we use links? (don't have strong opinion)
I think so. This is introductory, so someone may want to read up on the FFM API before diving deeper in jextract ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1567320384
On Mon, 15 Apr 2024 23:01:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 70:
68: [library loading](#library-loading) for more information. 69: 70: Besides these options, it is also possible to filter the output of jextract using one of the `--include-XXX` options
Suggestion: Besides these options, it is also possible to filter the output of jextract using one of the `--include-XYZ` options ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566970447
On Mon, 15 Apr 2024 23:01:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 236:
234: ``` 235: 236: Jextract will generate a set of simple getter methods to access the constant values of the
Nit: maybe omit the word `set` - e.g. just "generate simple getter methods...". Seeing "set" so close to "getter" might be confusing. doc/GUIDE.md line 262:
260: through the FFM API. 261: 262: Note that for macros, jextract only generates an accessor when it sees a macro definition,
Feels like here we need a link to the preprocessor section doc/GUIDE.md line 376:
374: ``` 375: 376: The pointer that is returned by the corresponding method that jextract generates
Consider streamlining this by saying "The pointer that is returned by new_point does not have..." doc/GUIDE.md line 378:
376: The pointer that is returned by the corresponding method that jextract generates 377: for this function does not have the correct bounds or lifetime associated with it. 378: These things are not possible to figure out automatically (for instance, a pointer
Suggestion: These properties cannot be inferred: for instance, a pointer... doc/GUIDE.md line 426:
424: ``` 425: 426: We again have a meta-data accessor for the function descriptor (`descriptor()`). There's
Maybe here using numbers in the code and a numbered list would be (a) more consistent and (b) more readable doc/GUIDE.md line 682:
680: 681: - Certain C types bigger than 64 bits (e.g. `long double` on Linux). 682: - Function-like macros (as mentioned in the [section on constants](#constants-macros--enums))
I wonder if here we got the reference backwards - e.g. perhaps the section on constants should just say that function-like macros are not supported and point here, then here we could say what workarounds could be used in more details? ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566978455 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566983513 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566987029 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566987831 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566990577 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566981579
On Tue, 16 Apr 2024 08:45:12 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 682:
680: 681: - Certain C types bigger than 64 bits (e.g. `long double` on Linux). 682: - Function-like macros (as mentioned in the [section on constants](#constants-macros--enums))
I wonder if here we got the reference backwards - e.g. perhaps the section on constants should just say that function-like macros are not supported and point here, then here we could say what workarounds could be used in more details?
Errno and endianness also come to mind in terms of unsupported stuff ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1567002714
On Mon, 15 Apr 2024 23:01:12 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
I've reviewed the document again. It looks very good, and the structure of the documents strikes me as just right (well done!). I've left picky comments (e.g. 99% review), which I consider optional. I'm ok with the PR being pushed in its current form. doc/GUIDE.md line 676:
674: In the above snippet, note that the load of the `baz` field value on the last line will 675: _see_ the update to the `bar` field of the `foo` instance on the line before. 676:
Perhaps we could also add a section (maybe in a followup PR) on fields/globals with array types, as jextract generates extra metadata (e.g. dimensions) and the Java type used for these accesses is MemorySegment (with bulk copy support on `set`) doc/GUIDE.md line 706:
704: ### Preprocessor Definitions 705: 706: C header files are processed by a pre-processor by a compiler before they are inspected
I think you mean pre-processor, and remove "by a compiler" ? ------------- Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jextract/pull/231#pullrequestreview-2003056787 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566997489 PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1566999472
On Tue, 16 Apr 2024 08:55:14 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
Update doc/GUIDE.md
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
doc/GUIDE.md line 676:
674: In the above snippet, note that the load of the `baz` field value on the last line will 675: _see_ the update to the `bar` field of the `foo` instance on the line before. 676:
Perhaps we could also add a section (maybe in a followup PR) on fields/globals with array types, as jextract generates extra metadata (e.g. dimensions) and the Java type used for these accesses is MemorySegment (with bulk copy support on `set`)
Yes, good point. This was missed ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1567357664
On Tue, 16 Apr 2024 13:24:44 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
doc/GUIDE.md line 676:
674: In the above snippet, note that the load of the `baz` field value on the last line will 675: _see_ the update to the `bar` field of the `foo` instance on the line before. 676:
Perhaps we could also add a section (maybe in a followup PR) on fields/globals with array types, as jextract generates extra metadata (e.g. dimensions) and the Java type used for these accesses is MemorySegment (with bulk copy support on `set`)
Yes, good point. This was missed
let's save this for a followup, and get this stake in the ground first :) (it will probably also be easier to review a small section) ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/231#discussion_r1567361099
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Update doc/GUIDE.md Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com> ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/5de1f4fd..b5ab52c9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=12 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with three additional commits since the last revision: - mor review comments - out-of-line javadoc links - more review comments ------------- Changes: - all: https://git.openjdk.org/jextract/pull/231/files - new: https://git.openjdk.org/jextract/pull/231/files/b5ab52c9..c3242781 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=13 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=231&range=12-13 Stats: 99 lines in 2 files changed: 37 ins; 8 del; 54 mod Patch: https://git.openjdk.org/jextract/pull/231.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/231/head:pull/231 PR: https://git.openjdk.org/jextract/pull/231
On Tue, 16 Apr 2024 13:36:35 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with three additional commits since the last revision:
- mor review comments - out-of-line javadoc links - more review comments
I've address the last of the review comments. I've also move all the javadoc links out of line, since they are so long (and it might be nice to have them in one place, should we need to update them in the future). I'll integrate this PR if there are no further comments. Thanks for the detailed review! ------------- PR Comment: https://git.openjdk.org/jextract/pull/231#issuecomment-2059117294
On Tue, 16 Apr 2024 13:36:35 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
Jorn Vernee has updated the pull request incrementally with three additional commits since the last revision:
- mor review comments - out-of-line javadoc links - more review comments
Marked as reviewed by mcimadamore (Reviewer). ------------- PR Review: https://git.openjdk.org/jextract/pull/231#pullrequestreview-2003728957
On Tue, 9 Apr 2024 17:20:22 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Add a comprehensive jextract guide under a new `doc/` folder.
This is meant as a comprehensive guide about the features of jextract and the code that it generates (including both examples from header files, the code that jextract generates, and corresponding Java user code). This is a first cut, and I'm anticipating quite a bit of comments.
Some sections of the readme have been moved to the guide (with minor edits here and there), and replaced by a link in the readme.
This pull request has now been integrated. Changeset: 7242f3e1 Author: Jorn Vernee <jvernee@openjdk.org> URL: https://git.openjdk.org/jextract/commit/7242f3e1fd48fd74a5b1af486b9f3c8baba8... Stats: 1112 lines in 2 files changed: 941 ins; 171 del; 0 mod Add jextract guide Reviewed-by: mcimadamore ------------- PR: https://git.openjdk.org/jextract/pull/231
participants (3)
-
Jorn Vernee
-
Maurizio Cimadamore
-
Nir Lisker