Skip to content

JENA-1519: OpWalkerVisitor with custom operators#394

Merged
asfgit merged 2 commits intoapache:masterfrom
jeremy-coulon:bug-opext-jira-1519
Apr 18, 2018
Merged

JENA-1519: OpWalkerVisitor with custom operators#394
asfgit merged 2 commits intoapache:masterfrom
jeremy-coulon:bug-opext-jira-1519

Conversation

@jeremy-coulon
Copy link

No description provided.

@Override
protected void visitExt(OpExt op) {
before(op) ;
super.visitExt(op);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this. It is visiting both the effective op and the real OpExt. Shoudn't it be the visitor deciding that? ie. visiting op.effectiveOp() is required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visiting the real OpExt is not useful for me but I can't say for other people.

Copy link
Member

@afs afs Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are probably both cases. Its an extension point so it is hard to be categorical. Only the effective is walkable unless the OpExt is comprised of known Ops in which case it does not need to be an OpExt.

Let's leave it as per the PR - we might have to return to it but without a concrete alternative usage, it feels like slipping towards guessing.

I would pull down the super impl and be explicit it walks the effectiveOp, not devolves the decision to the default method. This fixes the walker policy.

@asfgit asfgit merged commit 9c2bb1c into apache:master Apr 18, 2018
@jeremy-coulon jeremy-coulon deleted the bug-opext-jira-1519 branch April 18, 2018 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants