[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