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

SimpleNode: convenience builder functions #704

Merged
merged 1 commit into from
Jan 4, 2025
Merged

Conversation

caleridas
Copy link
Collaborator

Add constructor as well as helper function to create simple and its operator in a single call. Allow to move the created operator in (instead of copying it).

@caleridas caleridas requested a review from phate January 3, 2025 09:49
@caleridas
Copy link
Collaborator Author

I would have preferred different argumet ordering for the convenience function, i.e.:
CreateOpNode(, { <operands< })

but variadic template arguments must be last, and there are only hacky ways using lots of template magic to have it otherwise.

@phate phate requested a review from haved January 3, 2025 22:35
Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

A few rough corners that do not make sense to me, as well as some minor nitpicks that I do not expect to be fixed if you do not want to.

jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
@caleridas caleridas force-pushed the simple-node-builder branch 2 times, most recently from 3b8982c to 7be2356 Compare January 4, 2025 11:10
@caleridas caleridas requested a review from phate January 4, 2025 11:10
phate
phate previously approved these changes Jan 4, 2025
Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

Looks generally fine to me. One final thought: Would it be possible to move these functions into the SimpleNode class instead of having them as free standing functions?

Usage would then be something like this: SimpleNode::CreateOpNode<>(....)

I also do not particularly like the name CreateOpNode, but I guess a simple Create() or CreateNode() won't work as it would clash with the others already being there.

jlm/rvsdg/simple-node.hpp Outdated Show resolved Hide resolved
@caleridas
Copy link
Collaborator Author

Looks generally fine to me. One final thought: Would it be possible to move these functions into the SimpleNode class instead of having them as free standing functions?

Usage would then be something like this: SimpleNode::CreateOpNode<>(....)

I also do not particularly like the name CreateOpNode, but I guess a simple Create() or CreateNode() won't work as it would clash with the others already being there.

I would actually prefer free functions, I would prefer if everything in the nodes is concerned with their properties and access, but this convenience helper actually crosses "operator" and "node" so it is not exactly clear where it belongs to.

@caleridas caleridas force-pushed the simple-node-builder branch from 3048b62 to a8a507a Compare January 4, 2025 18:05
Copy link
Owner

@phate phate left a comment

Choose a reason for hiding this comment

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

I would actually prefer free functions, I would prefer if everything in the nodes is concerned with their properties and access, but this convenience helper actually crosses "operator" and "node" so it is not exactly clear where it belongs to.

Fair point.

@caleridas caleridas force-pushed the simple-node-builder branch from a8a507a to aa8b00d Compare January 4, 2025 20:46
Add constructor as well as helper function to create simple and its
operator in a single call. Allow to move the created operator in (instead of
copying it).
@caleridas caleridas force-pushed the simple-node-builder branch from aa8b00d to 69028cb Compare January 4, 2025 21:42
@caleridas caleridas merged commit 07884f1 into master Jan 4, 2025
12 checks passed
@caleridas caleridas deleted the simple-node-builder branch January 4, 2025 22: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.

2 participants