[foreign] RFR 8215211: cleanup JType/TypeDIctionary

[Maurizio Cimadamore]

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             }


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?

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