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 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 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 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 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: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 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 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 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 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 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: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: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 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: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 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

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: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: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 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 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 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 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: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

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 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: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 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 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 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: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