Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove typedtreeMap #2173

Merged
merged 3 commits into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@trefis
Copy link
Contributor

commented Nov 29, 2018

There are currently two typedtree mapper in the distribution: Tast_mapper and TypedtreeMap.
The first one is used in the compiler codebase, the second isn't.

Similarly, searching github for Tast_mapper returns 110 results in OCaml, while searching for TypedtreeMap returns only 60.

Equipped with these very scientifically obtained numbers, and the initial insightful observation, it seems obvious the TypedtreeMap should be the one to go.

trefis added some commits Nov 29, 2018

@trefis trefis force-pushed the trefis:tmapper branch from 59aec54 to 2017099 Nov 29, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Doing this, we end up with an inconsistent API with typedtreeIter on one hand and tast_mapper on the other, but no typedtreeMap or tast_iter -- this is not just a naming concern, the visitor-pattern style is fairly different on both ends, with typedtree{Map,Iter} being more stream-like and tast_{mapper,iter(ator?)} being a simpler open recursion. (And none of them is "shallow" like Btype.{iter,fold}_type_expr and the new Lambda iterators, and Flambda has completely different APIs again...)

On the typedtree we have both ast_{mapper,iterator}, consistent in design with tast_mapper (same authors), so it makes sense to keep tast_mapper indeed, but maybe we could try, medium term, to actually write tast_mapper for consistency and migrate the only use of TypedtreeIter (in our favorite parmatch.ml file) to it. Could you maybe create a Mantis issue with the "junior job" tag for this?

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

maybe we could try, medium term, to actually write tast_iterator for consistency and migrate the only use of TypedtreeIter (in our favorite parmatch.ml file) to it. Could you maybe create a Mantis issue with the "junior job" tag for this?

Sure, can do.
Although ideally, we would find a way to generate these iterators/mappers, instead of having to maintain them manually. (But that seems like a lot of work, and definitely not "junior" friendly).

Was your message a disguised approval of this PR?

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@Drup

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

Although ideally, we would find a way to generate these iterators/mappers, instead of having to maintain them manually. (But that seems like a lot of work, and definitely not "junior" friendly).

Janestreet mapper generators work very well on types/typedast. See: this mapper for types which was generated by ppx_traverse. I'm sure visitor would work as well.

When I was shopping for an visitor for the typedtree, I found typedtree{Map,Iter} to be barely usable at all, so moving to the style used by ppx/untypedast/* seems like a good direction to me.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

Do we really need to provide both mappers and iterators? One could just use an identity+side-effects mapper, it doesn't really complicate the client code that much and the impact on performance should be ok for most cases (if needed, one could also optimize the mapper to detect physical equality of all children before/aften the sub-rewriting and return the original term when nothing changed).

@gasche

gasche approved these changes Nov 29, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@Drup: I think we would want to stay away from objects for that part of the codebase.
(And to clarify, though I think you know that, the annoying part is how to set up the code generator, in a way that integrates well with the build system, the bootstrap cycle, etc.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

(Btw, the use of Tast_mapper in cmt2annot.ml is really an iterator.)

@trefis trefis merged commit af328e1 into ocaml:trunk Nov 29, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:tmapper branch Nov 29, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Do we really need to provide both mappers and iterators?

I think that it is better on the consumer side to be able to use the right abstraction for the job. It's fine if one is implemented in term of the other on the library side, but the problem here is that the interfaces are larger than the implementations, and do have to be duplicated. We have to balance the cost for the user of the abstractions, with the cost when modifying the type declarations, and I'm not sure what is the right choice.

gretay-js added a commit to gretay-js/ocaml that referenced this pull request Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.