Resolve #97: Allow `name` typed primary keys #98

nathanielhourt · 2023-01-27T04:58:43Z

Change Description

Resolve #97 -- Using a name as a primary key in a table is such a common use case that even the core contracts do it extensively. Adding first class support for this use case to the multi_index template is trivial, and allows contract code to be more expressive.

API Changes

API Changes

The multi_index API now supports usage of a name as a table primary key, and implicitly casts it down to an int for indexing.

Documentation Additions

Documentation Additions

dimas1185

please add 1 unit test with name as a primary key
optional but nice to have: reduce verbosity. use namespace at the beginning of methods and rename cast function to something concise like to_raw

_multi_index_detail::primary_key_cast(itm->primary_key()) //-->
using namespace _multi_index_detail;
to_raw(itm->primary_key()); // I would be happy to see some

dimas1185 · 2023-03-17T22:11:40Z

tests/unit/test_contracts/explicit_nested_tests.cpp

@@ -32,7 +32,7 @@ CONTRACT explicit_nested_tests : public contract {
      std::tuple<uint64_t, std::optional<float>, std::vector<int>> tup1;
      std::variant<uint64_t, std::optional<std::pair<int, float>>, std::vector<int>> var1;
      std::vector<std::vector<_mystruct>> vvmys;
-      auto primary_key() const { return id; }
+      auto primary_key() const -> decltype(id) { return id; }


if that is needed for every case we may break many contracts. Are you sure we need this?
If that is the case, please think of rewriting cast template so we can avoid this.

I honestly have no idea why this is necessary, but in multiple environments I and another dev see a build failure here unless the hint is provided. I am quite confident that standard C++ does not require the use of the hint here, but the peculiar Clang used to build contracts seems to get confused in this particular spot for some reason.

ScottBailey · 2023-03-18T00:24:59Z

please add 1 unit test with name as a primary key

optional but nice to have: reduce verbosity. use namespace at the beginning of methods and rename cast function to something concise like to_raw

100% agree with unit test.
Personally, I'm fine with seeing scope resolution in line - it makes grepping easier, for example, and makes it clear which namespace the symbol comes from.

I feel like to_raw() wouldn't be clear enough, if a shorter name is desired then maybe to_key() or to_primary_key() would be better?

ScottBailey

Add unit test, otherwise LGTM.

I'm not sure why this build failure occurs... It seems to me that the requisite `auto` should be resolvable by the time the table is used, but since it seems it is not, we give the compiler a hint to nudge it to the right answer. Using fancier syntax to make my problems go away =)

Create a unit test for a table with a name primary key. The new test fails to build before the commits in PReq AntelopeIO#98, but passes when those commits are included.

nathanielhourt · 2023-03-20T00:15:26Z

I have opted to shorted primary_key_cast to to_raw_pk, but leave the scope in place as it doesn't generally result in line wrapping, and in almost all cases, it only appears once per function so adding a using statement makes the code more verbose overall.

I've added a unit test which fails to build before the commits in this PReq, but which passes once they are added.

nathanielhourt · 2023-03-20T00:19:52Z

tests/unit/test_contracts/name_pk_tests.cpp

+    eosio::name pk;
+    int num;
+
+    auto primary_key() const { return pk; }


Calling attention from @dimas1185

I forgot to mention earlier, the return type hint is not generally necessary, so most contract code should be unaffected. There's just some peculiarity about the table in explicit_nested_tests.cpp that causes the hiccup.

In general, my changes do not cause failures around the concise primary_key() getter.

Create a unit test for a table with a name primary key. The new test fails to build before the commits in PReq #98, but passes when those commits are included.

nathanielhourt force-pushed the fix-name-pk branch from e401ac8 to b57ca6a Compare February 16, 2023 15:19

stephenpdeos requested review from dimas1185 and ScottBailey March 15, 2023 14:54

dimas1185 suggested changes Mar 17, 2023

View reviewed changes

dimas1185 reviewed Mar 17, 2023

View reviewed changes

ScottBailey suggested changes Mar 18, 2023

View reviewed changes

nathanielhourt and others added 3 commits March 18, 2023 22:53

Allow multi_index pk to be a name

b61ef08

[Name PK] Update accessors to allow name PK

e6977a1

nathanielhourt force-pushed the fix-name-pk branch from b57ca6a to 3485d33 Compare March 19, 2023 03:53

nathanielhourt added 3 commits March 19, 2023 18:29

Fix build: add missing scope resolution

cbb579d

Ref AntelopeIO#98: Shorten primary_key_cast to to_raw_pk

9e8385b

Ref AntelopeIO#97/AntelopeIO#98: Add unit test

faa9747

Create a unit test for a table with a name primary key. The new test fails to build before the commits in PReq AntelopeIO#98, but passes when those commits are included.

nathanielhourt commented Mar 20, 2023

View reviewed changes

nathanielhourt requested review from ScottBailey and dimas1185 and removed request for ScottBailey and dimas1185 March 20, 2023 06:13

dimas1185 approved these changes Mar 20, 2023

View reviewed changes

ScottBailey approved these changes Mar 20, 2023

View reviewed changes

ScottBailey merged commit 3d14a67 into AntelopeIO:main Mar 20, 2023

nathanielhourt deleted the fix-name-pk branch March 21, 2023 00:38

dimas1185 pushed a commit that referenced this pull request Jun 8, 2023

Ref #98: Shorten primary_key_cast to to_raw_pk

7cc450f

dimas1185 pushed a commit that referenced this pull request Jun 8, 2023

Ref #97/#98: Add unit test

732f83e

Create a unit test for a table with a name primary key. The new test fails to build before the commits in PReq #98, but passes when those commits are included.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve #97: Allow `name` typed primary keys #98

Resolve #97: Allow `name` typed primary keys #98

nathanielhourt commented Jan 27, 2023

dimas1185 left a comment

dimas1185 Mar 17, 2023 •

edited

Loading

nathanielhourt Mar 19, 2023

ScottBailey commented Mar 18, 2023

ScottBailey left a comment

nathanielhourt commented Mar 20, 2023

nathanielhourt Mar 20, 2023

Resolve #97: Allow name typed primary keys #98

Resolve #97: Allow name typed primary keys #98

Conversation

nathanielhourt commented Jan 27, 2023

Change Description

API Changes

Documentation Additions

dimas1185 left a comment

Choose a reason for hiding this comment

dimas1185 Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

nathanielhourt Mar 19, 2023

Choose a reason for hiding this comment

ScottBailey commented Mar 18, 2023

ScottBailey left a comment

Choose a reason for hiding this comment

nathanielhourt commented Mar 20, 2023

nathanielhourt Mar 20, 2023

Choose a reason for hiding this comment

Resolve #97: Allow `name` typed primary keys #98

Resolve #97: Allow `name` typed primary keys #98

dimas1185 Mar 17, 2023 •

edited

Loading