[foreign] RFR jextract should model clang Cursors as Trees

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Fri Aug 31 16:56:14 UTC 2018


Great work; I think this goes a long way towards putting some more solid 
foundation under jextract. It also allows, by putting most of the clang 
dependencies right into the immediate frontend, to make the rest of the 
code less sensitive to API changes (e.g. should we go from C to C++ 
clang API). Some comments below (some of those might be overly general 
and you might want to deal with them in followup changes):

* Thanks for adding the equals/hashcode method in Source[Location, Range]

* While it's probably early now, I think there are a lot of steps which 
might be translated into their own visitor - e.g. filtering cursors 
seems one of them (happens between Context and Parser)

* I also believe that, moving forward, classes such as Context and 
HeaderFile should be all about keeping state, and not about defining 
behavior (which something that should be delegated to tree visitors)

* I like that AsmCodeFactory just become a visitor!

* Macro handling became a bit better (since we now compute macros on 
parsing, and then delegate generation to the code factory) - I like 
that; but let's also keep in mind that macro computation can become its 
own tree step

* Nit: why is there a main method in ASMCodeFactory?

* It feels like inside Parser there's a tree printer (see main method). 
And that this printer should probably be unified with the main Printer 
class. And, pulling on this string some more, I'd like this printer 
exposed as its own class, so that we can use it to unit test parsing.

* It feels like we could decouple cursors from the tree even more; for 
instance, names are not reified in the AST (and types, too). This means 
that it is not possible, for instance, to define a visitor step which 
replaces some of the names (e.g. something like that would be needed for 
typedefs). The code, as now, will keep going back to the cursor.

* what exactly is CallbackTree? It seems unused (and so is one of the 
two createEnum overloads). I believe that is a leftover of some earlier 
attempts?

P.S.
Fetched patch, ran all tests, all clear on Linux x64.

Cheers
Maurizio


On 31/08/18 16:13, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210263
> Webrev: http://cr.openjdk.java.net/~sundar/8210263/webrev.00/
>
> Thanks,
> -Sundar



More information about the panama-dev mailing list