Visitor trait rewrite, step 1. #8527
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alpha-renamed top-level visit_* functions to walk_*. (Motivation: Distinguish visit action and recursive traversal.) Abstract over `&mut self` rather than over `@mut self`. This required some acrobatics, notably the `impl<E> Visitor<E> for @mut Visitor<E>` and corresponding introduction of `@mut Visitor` and some local `let mut` bindings. Remove oldvisit reference. Added default implementations for all of the Visitor trait methods. Note that both `visit_expr_post` and `visit_ty` are no-op's by default, just like they are in `oldvisit::default_visitor`. Refactoring: extract logic to ease swapping visit for oldvisit (hopefully).
placate make tidy. Remove dead code.
Placate make tidy. Remove unnecessary references to oldvisit.
fix borrowck/mod.rs to deal with fn_kind enum fallout.
|
Another thing I meant to mention: in this series of changes, I often chose to keep the diff's minimal even if it meant breaking rules about indentation, as code shifted from one context to another. After all this lands, follow-up commits can fix the whitespace (perhaps at the same time that they put in other transformations like replacing cases of But for these immediate pull requests, I wanted to ensure that these changes kept their nature as refactorings as obvious as possible. |
|
(also, I think there's clearly a latent bug in the |
|
Is there a minimized version of the |
|
@alexcrichton unfortunately i was not able to reproduce the problem in an isolated context. (I did try.) :( I'll give it another shot on Monday; I just didn't want to get distracted from the real goal here of getting this rewrite up for review. |
|
No problem, I can try to investigate once this lands as well. |
|
This looks great (if intermediate, as you said). I look forward to removing the legacy Visitor objects altogether and just using methods. |
bors
added a commit
that referenced
this pull request
Aug 15, 2013
…r=nikomatsakis Rewriting visit.rs to operate on a borrowed `&mut V` where `<V:Visitor>` r? @nikomatsakis r? @pcwalton This is the first in a planned series of incremental pull requests. (There will probably be five pull requests including this one, though they can be combined or split as necessary.) Part of #7081. (But definitely does *not* complete it, not on its own, and not even after all five parts land; there are still a few loose ends to tie up or trim afterwards.) The bulk of this change for this particular PR is pnkfelix/rust@3d83010, which has the changes necessary to visit.rs to support everything else that comes later. The other commits are illustrating the standard mechanical transformation that I am applying. One important point for nearly *all* of these pull requests: I was deliberately *not* trying to be intelligent in the transformation. * My goal was to minimize code churn, and make the transformation as mechanical as possible. * For example, I kept the separation between the Visitor struct (corresponding to the earlier vtable of functions that were potentially closed over local state) and the explicitly passed (and clones) visitor Env. I am certain that this is almost always unnecessary, and a later task will be to go through an meld the Env's into the Visitors as appropriate. (My original goal had been to make such melding part of this task; that's why I turned them into a (Env, vtable) tuple way back when. But I digress.) * Also, my main goal here was to get rid of the record of `@fn`'s as described by the oldvisit.rs API. (This series gets rid of all but one such case; I'm still investigating that.) There is *still* plenty of `@`-boxing left to be removed, I'm sure, and even still some `@fn`'s too; removing all of those is not the goal here; its just to get rid of the encoded protocol of `@fn`'s in the (old)visit API. To see where things will be going in the future (i.e., to get a sneak-preview of future pull-requests in the series), see: * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step1 (that's this one) * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step2 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step3 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step4 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step5 * Note that between step 4 and step 5 there is just a single commit, but its a doozy because its the only case where my mechanical transformation did not apply, and thus more serious rewriting was necessary. See commit pnkfelix/rust@da902b2
This was referenced Aug 15, 2013
bors
added a commit
that referenced
this pull request
Aug 19, 2013
…r=graydon,nikomatsakis r? @nikomatsakis Follow up to #8527 (which was step 1 of 5). See that for overall description Part of #7081
bors
added a commit
that referenced
this pull request
Aug 20, 2013
…r=huonw "non-mechanical" : there was lots more hacking than the other more-mechanical ports Felix did. r? @huonw. (Or @nikomatsakis ; I just want someone to sanity-check this. Its not a thing of beauty.) Followup to #8623. (See #8527, which was step 1 of 5, for the full outline. Part of #7081.) Notes on the change follow. There's also a strange pattern that I hacked in to accommodate the Outer/Inner traversal structure of the existing code (which was previously encoding this by untying the Y-combinator style knot of the vtable, and then retying it but superimposing new methods that "stop at items"). I hope either I or someone else can come back in the future and replace this ugliness with something more natural. Added boilerplate macro; all the OuterLint definitions are the same (but must be abstracted over implementing struct, thus the macro). Revised lint.rs use declarations to make ast references explicit. Also removed unused imports.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewriting visit.rs to operate on a borrowed
&mut Vwhere<V:Visitor>r? @nikomatsakis
r? @pcwalton
This is the first in a planned series of incremental pull requests. (There will probably be five pull requests including this one, though they can be combined or split as necessary.)
Part of #7081. (But definitely does not complete it, not on its own, and not even after all five parts land; there are still a few loose ends to tie up or trim afterwards.)
The bulk of this change for this particular PR is pnkfelix/rust@3d83010, which has the changes necessary to visit.rs to support everything else that comes later. The other commits are illustrating the standard mechanical transformation that I am applying.
One important point for nearly all of these pull requests: I was deliberately not trying to be intelligent in the transformation.
@fn's as described by the oldvisit.rs API. (This series gets rid of all but one such case; I'm still investigating that.) There is still plenty of@-boxing left to be removed, I'm sure, and even still some@fn's too; removing all of those is not the goal here; its just to get rid of the encoded protocol of@fn's in the (old)visit API.To see where things will be going in the future (i.e., to get a sneak-preview of future pull-requests in the series), see: