RFR: 7903615: Jextract should generate javadoc contents using libclang pretty printer [v3]

[Jorn Vernee]

On Thu, 21 Dec 2023 15:39:28 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR removes CDeclarationPrinter and, instead, uses libclang pretty-printing capabilities to print the javadoc contents for a given declaration.
>> 
>> The new logic will compute the comment string at parse time, in `TreeMaker`. The comment is then attached to the declaration using an attribute (and later retrieved when it's time to generate code).
>> 
>> I've used some custom printing policy options, after testing several of them to try and find the best results. With these options, there are two main differences from what we used to generate before:
>> 
>> 1. named structs/unions are re-expanded in function declarations (e.g. the javadoc of a function accepting a `struct Foo` will also contain the definition of `Foo`)
>> 2. there is no pretty-printing equivalent for types. This means that, for functional interfaces, we instead print the "parent" declaration, e.g. the declaration whose type is a function pointer.
>> 
>> It would be nice if we could improve on (1) by adding a new policy e.g. `IncludeAnonymousTagDefinition` - e.g. it is useful to have tag definitions inlined when the definition is anonymous (otherwise clang will generate a blob of text containing the source location of the anon definition), but it would be better perhaps not to inline the definition if it's not anonymous.
>> 
>> Example:
>> 
>> 
>> struct Point { int x; int y; };
>> 
>> void func(struct Point *pp);
>> 
>> 
>> This generates the following strings:
>> 
>> 
>> struct Point {
>>     int x;
>>     int y;
>> }
>> 
>> void func(struct Point {
>>     int x;
>>     int y;
>> } *pp)
>> 
>> 
>> I don't think it's a deal breaker, but it does make the javadoc more verbose.
>> 
>> Overall the changes are quite straightforward, and the code is simplified considerably. Also, this fixes a lot of little issues that arise when we try to generate javadoc for anonymous structs for which before only `struct` was printed.
>> 
>> There are only two situations in which I had to manually improve the clang output:
>> 
>> * macros - in this case pretty printer prints nothing :-) so I use the macro tokens to generate the corresponding doc string;
>> * enum constant - in this case pretty printer only prints the enum constant name, but the previous output had also the enum name and the value. I tweaked enum constants manually (also in `TreeMaker`). By doing that, I could then also remove `EnumConstantLifter`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix issue with comments in macro values

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 241:

> 239:         } else if (Utils.isPointer(type)) {
> 240:             // add pointer cast to make it look different from a numeric constant
> 241:             valueString = STR."(\{valueString})";

Shouldn't this be something like:


valueString = STR."({type}) {valueString}";


It seems like the current code would result in things like this:

    #define FOO (42)

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 483:

> 481:                     cursorString = declarationString(cursor, true);
> 482:                 }
> 483:                 yield cursorString;

So, if I understand directly, this is different from what is described in the PR body. e.g. we will try to expand struct types as just  `struct Foo`, unless we also find anonymous decls, in which case we expand to `struct Foo { int x; int y; }` ?

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 492:

> 490:     }
> 491: 
> 492:     String declarationString(Cursor cursor, boolean expandNestedDecls) {

Seems like these 2 methods could be `private` as well.

test/testng/org/openjdk/jextract/test/toolprovider/Test7903257/TestDocComments.java line 172:

> 170:         var comments = getDocComments("structs.h", "Point_t.java");
> 171:         assertEquals(comments, List.of(
> 172:             "typedef struct Point { int x; int y; } Point_t"));

I think this is probably better any way.

-------------

PR Review Comment: https://git.openjdk.org/jextract/pull/169#discussion_r1447361596
PR Review Comment: https://git.openjdk.org/jextract/pull/169#discussion_r1447366505
PR Review Comment: https://git.openjdk.org/jextract/pull/169#discussion_r1447363407
PR Review Comment: https://git.openjdk.org/jextract/pull/169#discussion_r1447346215