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
Attestation API updates for Electra #6557
Attestation API updates for Electra #6557
Changes from 7 commits
156f0ce
5a667d1
65ee98c
1115e4d
ecc9fa3
1d6aeb3
98013be
e298672
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.
default
is already atemplate
defined by the Nim standard library, so just as a point of naming, unless there's a quite compelling reason to name a symbol literallydefault
(or one is writing effectively an type-specialized implementation of thattemplate
) it's less confusing to use a different name; andvar default: seq[byte]
in general and then only use it in theexcept
part of thetry
/except
. One can just directly usedefault(seq[byte])
here and only incur the runtime cost on theexcept
case.I do see that various other things in
eth2_rest_serialization
adopt this pattern, but I don't see any reason to.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.
Nice one.
If I'm not mistaken "default" is a reserved keyword in several languages. Given that the whole file is using the same nomenclature, I didn't pay much attention to it.
The second point, I would say that the runtime cost is not a problem given that the whole exception mechanism is already a performance hit, but exception handling should be simple and mem allocation/deallocation should never fail.
Regarding your suggestion, template code will be injected/replace in place, however, what are the impacts of allocating a sequence on except scope? any leak possbility or anything that we might be missing nim-wise?
Neverthless, I'm adressing this on #6580