Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1963243003:
[compiler] Add a traversing AST visitor (Closed)
Description[compiler] Add a traversing AST visitor
Contrary to AstVisitor, which does not implement any traversal logic,
AstTraversalVisitor provides default implementations for each Visit*
function which walk through the AST. It is intended to be used as a base
class for visitors which are only interested in a small portion of the
AST.
R=yangguo@chromium.org
Committed: https://crrev.com/62b397a3a7d68d43f7cf93b655875205b3796259
Cr-Commit-Position: refs/heads/master@{#36283}
Patch Set 1 #Patch Set 2 : Compile fix for recent changes in master #Patch Set 3 : Remove accidental build file #
Messages
Total messages: 32 (12 generated)
Should we add someone else as reviewer for this one?
The CQ bit was checked by jgruber@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963243003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963243003/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
jgruber@google.com changed reviewers: + rossberg@chromium.org
On 2016/05/11 07:03:58, jgruber wrote: LGTM, though we should either use this soon, or add tests.
adamk@chromium.org changed reviewers: + adamk@chromium.org
This looks like it shares a lot of boilerplate with AstExpressionVisitor; could that be a subclass of this?
On 2016/05/11 22:38:20, adamk wrote: > This looks like it shares a lot of boilerplate with AstExpressionVisitor; could > that be a subclass of this? In general that would work, the big difference being that AstExpressionVisitor explicitly checks for stack overflow before and after it descends. I didn't add these checks to AstTraversalVisitor since they only make sense when surrounding code explicitly calls SetStackOverflow(). If we do that though, then making AEV a subclass would get rid of a bunch of duplicate code.
On 2016/05/12 08:12:20, jgruber wrote: > On 2016/05/11 22:38:20, adamk wrote: > > This looks like it shares a lot of boilerplate with AstExpressionVisitor; > could > > that be a subclass of this? > > In general that would work, the big difference being that AstExpressionVisitor > explicitly checks for stack overflow before and after it descends. I didn't add > these checks to AstTraversalVisitor since they only make sense when surrounding > code explicitly calls SetStackOverflow(). > > If we do that though, then making AEV a subclass would get rid of a bunch of > duplicate code. Having stack overflow checks in the new AstTraversalVisitor would actually make sense if we want to use it for other use cases. We could implement the AstExpressionVisitor by inheriting from AstTraversalVisitor, and override functions so that they visit the expressions and then calls the super method to continue traversal. Alternatively, we could implement the AstTraversalVisitor so that it traverses the AST, and at every AST node calls a delegate method to actually carry out the desired action. In AstTraversalVisitor the delegates for every AST node type would be no-ops. In the AstExpressionVisitor, we would override the delegate for those nodes where we need to call VisitExpression. For LiveEditFunctionVisitor we would override the delegate for FunctionLiteral. WDYT?
Ping :)
Description was changed from ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org BUG= ========== to ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org ==========
yangguo@chromium.org changed reviewers: + bmeurer@chromium.org
Benedikt, could you give this a look to unblock progress? Thanks.
lgtm
The CQ bit was checked by jgruber@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963243003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963243003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/5829) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/5805)
The CQ bit was checked by jgruber@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1963243003/#ps40001 (title: "Remove accidental build file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963243003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963243003/40001
Message was sent while issue was closed.
Description was changed from ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org ========== to ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org ========== to ========== [compiler] Add a traversing AST visitor Contrary to AstVisitor, which does not implement any traversal logic, AstTraversalVisitor provides default implementations for each Visit* function which walk through the AST. It is intended to be used as a base class for visitors which are only interested in a small portion of the AST. R=yangguo@chromium.org Committed: https://crrev.com/62b397a3a7d68d43f7cf93b655875205b3796259 Cr-Commit-Position: refs/heads/master@{#36283} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/62b397a3a7d68d43f7cf93b655875205b3796259 Cr-Commit-Position: refs/heads/master@{#36283}
Message was sent while issue was closed.
I'm a little confused; I read Yang's last question as being addressed to jgruber, but I don't see an answer to it on this patch. I'm worried that further duplication of this logic is likely to lead to bugs down the road as the visitors diverge.
Message was sent while issue was closed.
On 2016/05/18 19:24:14, adamk wrote: > I'm a little confused; I read Yang's last question as being addressed to > jgruber, but I don't see an answer to it on this patch. I'm worried that further > duplication of this logic is likely to lead to bugs down the road as the > visitors diverge. I assumed Yang's question was meant for you, which is why I was still waiting for an answer from you in fact :) I agree that we should not duplicate the visitor logic though and I'll take a look at this soon. Do you have an opinion on Yang's options above?
Message was sent while issue was closed.
On 2016/05/19 07:58:00, jgruber1 wrote: > On 2016/05/18 19:24:14, adamk wrote: > > I'm a little confused; I read Yang's last question as being addressed to > > jgruber, but I don't see an answer to it on this patch. I'm worried that > further > > duplication of this logic is likely to lead to bugs down the road as the > > visitors diverge. > > I assumed Yang's question was meant for you, which is why I was still waiting > for an answer from you in fact :) I agree that we should not duplicate the > visitor logic though and I'll take a look at this soon. Do you have an opinion > on Yang's options above? Yes indeed. I was trying to get an opinion from Adam on how to avoid code duplication.
Message was sent while issue was closed.
On 2016/05/19 08:01:45, Yang wrote: > On 2016/05/19 07:58:00, jgruber1 wrote: > > On 2016/05/18 19:24:14, adamk wrote: > > > I'm a little confused; I read Yang's last question as being addressed to > > > jgruber, but I don't see an answer to it on this patch. I'm worried that > > further > > > duplication of this logic is likely to lead to bugs down the road as the > > > visitors diverge. > > > > I assumed Yang's question was meant for you, which is why I was still waiting > > for an answer from you in fact :) I agree that we should not duplicate the > > visitor logic though and I'll take a look at this soon. Do you have an opinion > > on Yang's options above? > > Yes indeed. I was trying to get an opinion from Adam on how to avoid code > duplication. Apologies for the miscommunication, I should have stated my assumption when jgruber pinged. Will consider Yang's suggestion more, at first glance it seems pretty reasonable. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
