RFR: 7903605: Simplify TypeMaker

[Jorn Vernee]

On Fri, 8 Dec 2023 22:10:51 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> The main goal of this PR is to make TypeMaker simpler, and remove the convoluted logic which stashes pointers, and asks client to call a method to resolve their types.
> 
> Instead, this patch tweaks the behavior of the TypeMaker so that when a pointer to a declared type is created, it points to a special supplier. This supplier, when called, looks in the declaration cache for a matching declaration (in the `TreeMaker`). If a corresponding declaration is found, a new declared type pointing to that declaration is returned as a pointee type.
> 
> It is also possible for a pointer to point to an opaque struct, in which case there might be no symbol in the declaration cache. In such case we make the supplier return an erroneous type instead.
> 
> The patch also addresses a number of related issues:
> 
> * Since declarations are stable identity-wise but types are not, I removed the `Attributed` interface and corresponding implementation class. Now all the methods to deal with attributes are in `Declaration` - declarations are the only entities that can be reliably attributed (because of de-duplication).
> 
> * I added a mechanism to create "nested" `TreeMaker` - that is, a `TreeMaker` that can delegate a symbol lookup to a parent `TreeMaker`. This is handy when dealing with macros, where we do not want to pollute the original environment, but we still want to be able to resolve declarations included in that environment.
> 
> * I have fixed the equals methods in Declaration - these methods ignored attributes and were not working correctly. As a result, I also did some changes in duplicate filter, as using declaration equality does not work (because attributes might mismatch thanks to the added `Skip`).

Marked as reviewed by jvernee (Committer).

src/main/java/org/openjdk/jextract/Declaration.java line 136:

> 134:              * Class declaration.
> 135:              */
> 136:             CLASS,

Spurious change? Or is this needed now?

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

> 132:             case EnumDecl -> createEnum(c);
> 133:             case EnumConstantDecl -> createEnumConstant(c);
> 134:             case FieldDecl -> createVar(c, Declaration.Variable.Kind.FIELD);

FWIW, I see the `FieldDecl` case as being never executed in the code coverage report. Not sure why...

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

> 279:                         // process struct recursively
> 280:                         pendingFields.add(recordDeclaration(parent, fc).tree());
> 281:                     } else if (!fc.isBitField() && !fc.spelling().isEmpty()) {

Spurious change? Or was this condition needed after all?

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

> 361:             try {
> 362:                 pointeeType = ((Type.Delegated)canonicalType).type();
> 363:             } catch (IllegalStateException unresolved) {

This catch block is never taken according to coverage report. Maybe it can just be removed?

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

PR Review: https://git.openjdk.org/jextract/pull/155#pullrequestreview-1773272092
PR Review Comment: https://git.openjdk.org/jextract/pull/155#discussion_r1421091366
PR Review Comment: https://git.openjdk.org/jextract/pull/155#discussion_r1421110339
PR Review Comment: https://git.openjdk.org/jextract/pull/155#discussion_r1421110677
PR Review Comment: https://git.openjdk.org/jextract/pull/155#discussion_r1421111885