Skip to content
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

#2099: Make NodeType a strong type and use it across the codebase #2139

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

JacobDomagala
Copy link
Contributor

Fixes #2099

@JacobDomagala JacobDomagala force-pushed the 2099-use-strong-node-type branch 2 times, most recently from 71381b4 to 5a3e388 Compare April 26, 2023 20:24
@github-actions
Copy link

github-actions bot commented Apr 26, 2023

Pipelines results

PR tests (gcc-12, ubuntu, mpich)

Build for b8e2123 (2024-01-18 20:11:01 UTC)

FAILED: src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx.o 
/usr/bin/ccache /usr/lib/ccache/g++ -DJSON_USE_IMPLICIT_CONVERSIONS=1 -DVT_NO_COLOR_ENABLED -I/vt/lib/CLI -I/build/vt/release -I/vt/src -I/vt/lib/json/include -I/vt/lib/brotli/c/include -I/vt/lib/libfort/lib -isystem /vt/lib/fmt/include -isystem /vt/lib/EngFormat-Cpp/include -isystem /build/checkpoint/install/include -O3 -DNDEBUG -Wall -pedantic -Wshadow -Wno-unknown-pragmas -Wsign-compare -ftemplate-backtrace-limit=100 -Werror -std=c++17 -MD -MT src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx.o -MF src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx.o.d -o src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx.o -c /build/vt/src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx
In file included from /vt/src/vt/group/region/group_list.h:47,
                 from /vt/src/vt/objgroup/proxy/proxy_objgroup.h:62,
                 from /vt/src/vt/objgroup/manager.h:52,
                 from /vt/src/vt/runnable/runnable.cc:45,
                 from /build/vt/src/CMakeFiles/vt.dir/Unity/unity_19_cxx.cxx:3:
/vt/src/vt/group/region/group_region.h: In member function 'size_t vt::group::region::ListHash::operator()(const vt::group::region::Region::ListType&) const':
/vt/src/vt/group/region/group_region.h:87:27: error: no match for call to '(std::hash<vt::util::strong::detail::Strong<short int, -1, vt::StrongNodeType> >) (const short int&)'
   87 |             seed ^= hasher(i) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
      |                     ~~~~~~^~~
In file included from /vt/src/vt/epoch/epoch_type.h:47,
                 from /vt/src/vt/configs/types/types_headers.h:52,
                 from /vt/src/vt/configs/debug/debug_print.h:47,
                 from /vt/src/vt/configs/debug/debug_masterconfig.h:55,
                 from /vt/src/vt/config.h:47,
                 from /vt/src/vt/messaging/message/smart_ptr.h:47,
                 from /vt/src/vt/runnable/runnable.h:47,
                 from /vt/src/vt/runnable/runnable.cc:44:
/vt/src/vt/utils/strong/strong_type.h:347:10: note: candidate: 'std::size_t std::hash<vt::util::strong::detail::Strong<T, init_val, Tag> >::operator()(const vt::util::strong::detail::Strong<T, init_val, Tag>&) const [with T = short int; T init_val = -1; Tag = vt::StrongNodeType; std::size_t = long unsigned int]'
  347 |   size_t operator()(
      |          ^~~~~~~~
/vt/src/vt/utils/strong/strong_type.h:348:63: note:   no known conversion for argument 1 from 'const short int' to 'const vt::util::strong::detail::Strong<short int, -1, vt::StrongNodeType>&'
  348 |     vt::util::strong::detail::Strong<T, init_val, Tag> const& in
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
FAILED: src/CMakeFiles/vt.dir/Unity/unity_18_cxx.cxx.o 
/usr/bin/ccache /usr/lib/ccache/g++ -DJSON_USE_IMPLICIT_CONVERSIONS=1 -DVT_NO_COLOR_ENABLED -I/vt/%0D%0A%0D%0A%0D%0A ==> And there is more. Read log. <==

Build log


@JacobDomagala
Copy link
Contributor Author

JacobDomagala commented Apr 26, 2023

vt-sample has to be updated

@JacobDomagala
Copy link
Contributor Author

@lifflander Do we want to support arithmetic operations on the new Node type? (i.e. NodeType / x; NodeType % x; NodeType * x), or should we only permit StrongType versions? (i.e.NodeType{x} / NodeType{y}; NodeType{x} % NodeType{y}; NodeType{x} * NodeType{y}

@lifflander
Copy link
Collaborator

Yes, I do think we should support arithmetic operations.

@JacobDomagala JacobDomagala force-pushed the 2099-use-strong-node-type branch from 8e1fb03 to 9b434a6 Compare May 8, 2023 18:13
@JacobDomagala
Copy link
Contributor Author

We probably would want to come up with a way to not break vt dependent code that uses old NodeType

@JacobDomagala JacobDomagala self-assigned this Sep 12, 2023
@JacobDomagala JacobDomagala force-pushed the 2099-use-strong-node-type branch from 9b434a6 to b8e2123 Compare January 18, 2024 20:11
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.

Use strong Node type across code base to prevent confusion
2 participants