Skip to content

Conversation

@xDimon
Copy link
Member

@xDimon xDimon commented Dec 13, 2024

Update code of parsing asn1 files in according with changes of jam-test-vectors.
Fix safrol test.

@xDimon xDimon requested review from kamilsa and turuslan December 13, 2024 14:02

namespace jam {

template <typename T, typename = std::enable_if<std::is_scalar_v<T>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use Foo = Tagged<T> instead of struct Foo { T v }?

Why inherit Tagged<T> : T {} instead of Tagged<T> { T v }?

Copy link
Member Author

@xDimon xDimon Dec 30, 2024

Choose a reason for hiding this comment

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

Why use Foo = Tagged instead of struct Foo { T v }?

Composition hides public members, and developers have to have access to them over composed member. It makes code a bit noisy and non clear. Tagged types more transparent.

Why inherit Tagged : T {} instead of Tagged { T v }?

Same explanation. But additionally for POD type is used zero-cost Wrapper.

/// Special zero-size-type for some things
/// (e.g., dummy types of variant, unsupported or experimental).
template <size_t N>
using Unused = Tagged<Empty, NumTag<N>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate struct, not related to Empty (encode/decode)

Suggested change
using Unused = Tagged<Empty, NumTag<N>>;
struct Unused {};

Copy link
Member Author

Choose a reason for hiding this comment

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

No. std::variant can not contain two same type. Tagged type resolves this disadvantage.

#include <test-vectors/vectors.hpp>

namespace jam::test_vectors_disputes {
namespace jam::test_vectors::disputes {
Copy link
Contributor

Choose a reason for hiding this comment

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

namespace test_vectors_disputes was intentional, to avoid
"no type/function foo in namespace disputes, did you mean jam::disputes::foo or jam::test_vectors::disputes::foo" error

// TODO(turuslan): #3, wait for test vectors
.iota = iota,
.gamma_a = gamma_tick_a,
.gamma_a = {gamma_tick_a.begin(), gamma_tick_a.end()},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.gamma_a = {gamma_tick_a.begin(), gamma_tick_a.end()},
.gamma_a = asVec(gamma_tick_a),

Copy link
Member Author

Choose a reason for hiding this comment

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

I may be wrong, but I think in my implementation vector emplaced by iterators, but asVec creates temporary vector and moves it.

@xDimon xDimon merged commit 00211c1 into master Dec 30, 2024
2 checks passed
@xDimon xDimon deleted the update/asn1_files branch December 30, 2024 07:59
xDimon added a commit that referenced this pull request Jun 6, 2025
* update: submodule jamtestvectors

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

* draft

* draft*

* draft

* draft

* draft

* fix

* draft

* draft

* fix: safrol test

* fix: safrol test

* update: test-vectors from upstream

* fix: review issues

* fix: review issue

---------

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
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.

4 participants