Skip to content

Conversation

@greg7mdp
Copy link
Member

@greg7mdp greg7mdp commented Oct 18, 2024

This is an experiment attempting to make two members of action_base const.

We likely do not want to merge the const change as it may cause other repo tests to fail (change in second commit 986562e). However this commit was removed in the current version of the PR.

I propose that we merge the PR as it currently is. It does not change action_base, but updates our code to construct action using the constructor, instead of declaring a default constructed object and assigning the members after the fact.

There should be no functional difference, besides constructing action with the constructor instead of individual member assignments.

@greg7mdp greg7mdp requested review from heifner, linh2931 and spoonincode and removed request for linh2931 and spoonincode October 18, 2024 16:18
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we found no way to regenerate these files properly? I think if anything should update the generator,

func << " action test;\n";
func << " test.account = \"wasmtest\"_n;\n";
func << " test.name = account_name((uint64_t)index);\n";
func << " test.authorization = {{\"wasmtest\"_n, config::active_name}};\n\n";
func << " push_action(tester, std::move(test), \"wasmtest\"_n.to_uint64_t());\n";

btw it sure looks like AntelopeIO/wasm-spec-tests@026fb4b found a way to regenerate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the generator b158e5b.

act.data = abi_ser.variant_to_binary(action_type_name, data, abi_serializer::create_yield_function( T::abi_serializer_max_time ));
action act(vector<permission_level>{{signer, config::active_name}}, "eosio.token"_n, name,
abi_ser.variant_to_binary(action_type_name, data, abi_serializer::create_yield_function( T::abi_serializer_max_time )));

Copy link
Member

Choose a reason for hiding this comment

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

I feel the original way of initializing individual fields explicitly clearer and more readable. action is a data class and used most locally. It is probably appropriate to be a struct without const.

Copy link
Member Author

@greg7mdp greg7mdp Oct 18, 2024

Choose a reason for hiding this comment

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

I very much disagree. Because action has constructors, it implies that the object should be constructed using the provided constructors.

The direct member initialization forgoes any hope to ensure that objects are always constructed in a consistent fashion, and that some internal members (possibly dependent on the constructors parameters) are always initialized correctly. This is not the case for action, but I feel it is a better pattern to follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

@arhag do you have an opinion on whether it is worthwhile to merge this PR?

@greg7mdp greg7mdp requested a review from linh2931 October 21, 2024 20:09
@ericpassmore
Copy link
Contributor

Note:start
category: Chores
component: Internal
summary: Improve constructor by using action instead of assigning members after the fact.
Note:end

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.

6 participants