[foreign] RFR 8215211: cleanup JType/TypeDIctionary

[Sundararajan Athijegannathan]

+1


inline comments below..

On 12/12/18, 3:59 PM, Maurizio Cimadamore wrote:
>
> Thanks for the review, comment below:
>
> On 12/12/2018 08:43, Sundararajan Athijegannathan wrote:
>> Very nice! I built & tested the patch on Mac and also ran our 
>> platform specific samples (on mac). All fine.
>>
>> One question: why is the EnumTree part in typedef handler is removed?
>
> The new code is this:
>
>   else if (defTree.name().equals(tt.name())) {
>   189                 /*
>   190                  * Remove redundant typedef like:
>   191                  *
>   192                  * typedef struct Point { int x; int y; } Point
>   193                  * typedef enum Color { R, G, B} Color
>   194                  * typedef struct Undef Undef
>   195                  */
>   196                 return null;
>   197             }

Ah.. right! Missed the proper diff :)

>
> That is, if the typedef name matches the name of the entity being 
> typedef'ed, then the typedef should always be considered redundant, 
> regardless of it being an enum, struct, ...
>
> The old code was doing something similar, but had two different paths 
> for structs and enums - the new code unifies the 'redundant check' 
> logic. It think it should be functionally equivalent to what we had.
>
> Question: this visitor really does two things:
>
> 1) removes redundant type def, as in 'typedef struct Foo { } Foo'
>
> 2) move name from typedef to anon entity 'typedef struct { } Foo'
>
> We discussed about (1). Now, about (2), the code only does the name 
> injection if we have an anon struct - can we also have an anon enum? 
> E.g. might it make sense to apply both (1) and (2) regardless of the 
> entity being typedef'ed?
>

We don't have any explicit "enum" type mapped to Java -bunch of getters 
for "enum constants" in the containing header interface. typedef name 
propagation for enum may not help much - except perhaps for the 
annotation for enum.  That seems to happen already.

I tried the following:

typedef enum {
    R, G, B
} Color;

Color annotation is generated in this case...

Thanks,
-Sundar

> Maurizio
>
>
>> Other than that +1
>>
>> -Sundar
>>
>> On 11/12/18, 9:32 PM, Maurizio Cimadamore wrote:
>>> Hi,
>>> please review this jextract patch:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8215211_v2/
>>>
>>> The goal is to simplify the JType hierarchy and the TypeDictionary 
>>> class. JType is currently a very rich hierarchy that is capable of 
>>> modeling basically any clang type/cursor. In a way, JType is 
>>> overlapping a lot with the recently added jextract tree 
>>> representation; it feels like JType can be greatly simplified, and 
>>> we can just rely on trees for structural info.
>>>
>>> Simplifying the JType hierarchy has many benefits - in the patch you 
>>> will notice that we no longer need a lot of instance tests and casts.
>>>
>>> The second issue is with TypeDictionary, whose lookup/define logic 
>>> is scattered across TypeDictionary and HeaderFile::define. There's 
>>> also a very complex lookup scheme which starts from 
>>> TypeDictionary::lookup and then ends up in Context then back to 
>>> HeaderFile::define. This is all very hard to follow.
>>>
>>> I've now centralized all type resolution logic in TypeDictionary, 
>>> which now features two functions: lookup and enterIfAbsent, which 
>>> internally share the same 'getInternal'. TypeDictionary will simply 
>>> create a JType from a clang Type, and the only thing a 
>>> TypeDictionary has to keep track of, is the use of the anonymous 
>>> function types created during the 'enter' phase. Note that such 
>>> enter phase has now been moved into a proper tree visitor.
>>>
>>> This simplify things quite a bit, to the point that 
>>> AsmCodeFactory::addType becomes a simple visitor call. I think more 
>>> rounds of cleanup are possible, but, to get there, we need to 
>>> simplify the logic of Context/HeaderFile::processTree, which 
>>> requires having a more precise mapping between clang compilation 
>>> units and jextract trees. For instance, we currently parse 
>>> everything into the same 'header tree', even if we end up generating 
>>> multiple classes; secondly, we sorely miss a CallbackTree, so we 
>>> have to do a lot of gymnastic in order to understand which 
>>> classfiles need to be generated for functional interfaces.
>>>
>>> Note: I seem to recall that function pointer typedef should emit, a 
>>> named functional interface matching the name of the typedef. But the 
>>> I noted that this is not the way things work in the current impl, so 
>>> I did not bother too much with that (but I can address that if 
>>> required, although doing so will cause breakages in tests, so I'd 
>>> prefer to do that separately).
>>>
>>> Cheers
>>> Maurizio
>>>