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.
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
Yul: Introduces ASTNodeRegistry #15823
base: develop
Are you sure you want to change the base?
Yul: Introduces ASTNodeRegistry #15823
Changes from all commits
40c03b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situation would you need to print these ghost labels? Can we just assume that it will never happen and assert against it instead?
If you do need to print them, you should at least ensure that they cannot conflict with existing labels. I see that
define()
andASTNodeRegistryBuilder
constructor allow arbitrary identifiers, but it seems that the implicit assumption is that they will only ever get valid Yul identifiers. That should be documented and it would be good to at least assert that the value does not contain any[
characters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, currently
define()
will accept a label like"GHOST[@]"
and just give you the ID of the ghost placeholder instead of defining a label with that exact text, which I think is unexpected. IMO it should trigger an assert and if inserting ghosts is an intended use case, there should be a dedicated method for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tthey are attached to the CFG (not the regular AST) and could occur, e.g., in error messages and also in the dotgraph output of CFGs.
In general I would like to avoid that assumption. The ghost stuff is a bit of an corner case which was handled pretty much the same way with
YulStrings
, just that now the id is the hash. I do like the suggestion though to takenumeric_limits<size_t>::max()
as ghost base-id and simply not have the placeholder, it should not be necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You described these classes in the PR description, but I'd much rather just have docstrings for them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially
m_idToLabelMapping
andm_labels
should be documented. Like, what are the numbers stored inm_idToLabelMapping
and the assumptions about them (can there be duplicates? do both vectors have to have same length?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some docs - let me know what you think and/or if it should be expanded somewhere. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I still don't fully understand is the purpose of the ghost IDs. Looking at other PRs, I see the mechanism for adding them, but not yet any place that would actually add them. The intended usage is a detail that the docstring should explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ghost nodes are added during CFG construction. They only live in the CFG itself and are not actually referenced in the AST. We could potentially also remove them here and specialize them out for CFGs. Then it's more local to where they are introduced and needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be
ID
for consistency with other acronyms likeAST
:Here and generally in all the other names.
I'm also still not sure
NodeID
andASTNodeRegistry
really reflect the purpose well. I see that in AST you even call the registrylabels
. Wouldn'tLabelID
andASTLabelRegistry
fit better? I don't think we're really using it as a node ID. We have some node types without any and technically we could also define some nodes where having more than one would make sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though TBH this still does not say everything I wanted to know when I started reviewing this. That's how I'd put it myself:
But note that it includes some of my assumptions how it should work, which may or may not be true depending on the answers to my earlier comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I think we should make
m_labels
andm_idToLabelMapping
const
. The container is immutable so they're not supposed to ever change after initialization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll get rid of implicit copy/move, though. The immutability is reflected by it not having any non-const methods. That would already take care of that unless one const-casts.