Skip to content

operands init #202

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

Closed
wants to merge 8 commits into from
Closed

operands init #202

wants to merge 8 commits into from

Conversation

dannypsnl
Copy link
Member

resolve #191

@dannypsnl dannypsnl requested a review from mewmew August 21, 2021 01:28
@dannypsnl
Copy link
Member Author

@mewmew updated

Copy link
Member

@mewmew mewmew left a comment

Choose a reason for hiding this comment

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

Good job on this. We should decide on the API of Operators. Should it return a mutable list of operands or not? If it should return a mutable list, then the return type should be updated to []*value.Value.

@dannypsnl dannypsnl requested a review from mewmew September 16, 2021 16:06
@mewmew
Copy link
Member

mewmew commented Sep 16, 2021

Hi @dannypsnl,

As this will be part of the core API which defines how instructions are used, I think it is important that we take time to consider use cases and get the design just right for the Operands method. (this is in relation to commit f568d83)

I am leaning towards having Operands return a mutable list, as it allows for a very powerful API; see #191 (comment) for further background. This would also help resolve (at least part of) the use cases detailed in #42.

Regarding the first point, I think this will allow for a very powerful API. Similar to the IR traversal API (inspired by Go fix) that we have in irutil.Walk.

So. I would like for us to take time and consider the benefit vs. drawbacks of having a mutable list returned by the Operands type, as this may also be used for other data analysis APIs in the future, e.g. use-def chains, etc.

For the time being, those who need to start using the Operands API today before we finalize it may do so by pinning the Git commit hash of the operands branch (e.g. by doing go get -u github.com/llir/llvm@operands in a Go module enabled repository).

Cheers,
Robin

@dannypsnl
Copy link
Member Author

Since value.Value is an interface, does mutation really work on it?

@mewmew
Copy link
Member

mewmew commented Dec 13, 2021

Since value.Value is an interface, does mutation really work on it?

No, this is why we would need to use []*value.Value for mutation.

Note, the AST walker of the go fix tool relies on pointer to interface to mutate Go source code by AST traversal. It provides a really powerful API!

From golang/go/src/cmd/fix/walk.go:

	switch n := x.(type) {
	// pointers to interfaces
	case *ast.Decl:
		walkBeforeAfter(*n, before, after)
	case *ast.Expr:
		walkBeforeAfter(*n, before, after)
	case *ast.Spec:
		walkBeforeAfter(*n, before, after)
	case *ast.Stmt:
		walkBeforeAfter(*n, before, after)

	// pointers to struct pointers
	case **ast.BlockStmt:
		walkBeforeAfter(*n, before, after)
	case **ast.CallExpr:
		walkBeforeAfter(*n, before, after)

	// etc...
	}

@dannypsnl dannypsnl closed this Dec 25, 2021
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.

interface BinaryInstruction
2 participants