Use std::array in base_uint instead of C-style array #2514

seelabs · 2018-05-09T12:57:06Z

This is the new PR for #2509

seelabs · 2018-05-09T12:59:15Z

This looks good to me. 👍. Thanks @JoeLoser!

ripplelabs-jenkins · 2018-05-09T13:35:38Z

Jenkins Build Summary

Built from this commit

Built at 20180531 - 16:35:04

Test Results

Build Type	Log	Result	Status
docs	logfile	1 cases, 0 failed, t: 2s	PASS ✅
msvc.debug	console	no test results, t: n/a	FAIL 🔴
msvc.debug, PROJECT_NAME=rippled_classic	console	no test results, t: n/a	FAIL 🔴
msvc.release	console	no test results, t: n/a	FAIL 🔴
clang.debug.unity	logfile	1030 cases, 0 failed, t: 250s	PASS ✅
coverage	logfile	1030 cases, 0 failed, t: 620s	PASS ✅
clang.debug.unity, PARALLEL_TESTS=false	logfile	1030 cases, 0 failed, t: 489s	PASS ✅
gcc.debug.unity	logfile	1030 cases, 0 failed, t: 242s	PASS ✅
clang.debug.nounity	logfile	1028 cases, 0 failed, t: 657s	PASS ✅
clang.release.unity	logfile	1029 cases, 0 failed, t: 331s	PASS ✅
gcc.debug.nounity	logfile	1028 cases, 0 failed, t: 667s	PASS ✅
gcc.debug.unity -Dstatic=true	logfile	1030 cases, 0 failed, t: 239s	PASS ✅
gcc.release.unity	logfile	1029 cases, 0 failed, t: 346s	PASS ✅

codecov-io · 2018-05-09T13:35:53Z

Codecov Report

Merging #2514 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2514      +/-   ##
===========================================
- Coverage    70.66%   70.64%   -0.02%     
===========================================
  Files          696      696              
  Lines        54405    54405              
===========================================
- Hits         38444    38437       -7     
- Misses       15961    15968       +7

Impacted Files	Coverage Δ
src/ripple/basics/base_uint.h	`98.9% <100%> (ø)`	⬆️
src/ripple/basics/DecayingSample.h	`77.77% <0%> (-8.34%)`	⬇️
src/ripple/server/impl/BaseHTTPPeer.h	`87.03% <0%> (-3.09%)`	⬇️
src/ripple/core/impl/Workers.cpp	`100% <0%> (+1.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d702e73...d80d74a. Read the comment docs.

HowardHinnant · 2018-05-09T16:48:31Z

src/ripple/basics/base_uint.h

@@ -419,7 +419,7 @@ class base_uint

    base_uint<Bits, Tag>& operator=(Zero)
    {
-        memset (&pn[0], 0, sizeof (pn));
+        memset (pn.data (), 0, sizeof (pn));


This can now be:

pn.fill(0);

Similarly, does the need for compare (at line 435) go away, and we can implement comparisons by delegating to the comparison operators for std::array?

@nbougalis Sorry for the long latency response w/ CppNow going on. As far I can tell,

We can remove compare in base_uint.h and delegate like you suggested just by making the comparison ops friends.

For fixing the call sites that were relying on this compare function being available to define their own ordering (such as Issue.cpp), we can then just use the comparison operators directly.

This weekend, I should have some free time to clean up this PR and I'll post a proposed patch then.

After digging into this for a little bit, I'd prefer to punt on this for now. Implementing the comparison operators just as delegating to the std::array comparison ops didn't "just work". Plus, it looks like we pass this compare function around as a function object in a different place (TxMeta.cpp).

So, my preference would be that I'll create a GitHub issue to capture this idea but do not wish to couple it with this PR, even though it may naturally seem like a good candidate.

Edit: created an issue

HowardHinnant · 2018-05-09T16:54:09Z

The copy assignment operator can now be = default. And we can either let the compiler implicitly declare both the copy constructor and copy assignment, or but both of their = default declarations next to each other. I have a mild preference for the former (less noise).

JoeLoser · 2018-05-09T17:30:43Z

@HowardHinnant thanks for the feedback. I'll make those good changes tonight and send a patch up for @seelabs to pull in to this. There may be other places to leverage simple standard algorithms too, so I'll look into that.

HowardHinnant · 2018-05-09T20:10:50Z

There's a assert(sizeof(pn) == bytes) which should be a static_assert(sizeof(pn) == bytes, "") and can be moved to class scope, perhaps just after the bytes definition.

nbougalis · 2018-05-09T21:16:51Z

src/ripple/basics/base_uint.h

@@ -314,7 +314,7 @@ class base_uint
        Hasher& h, base_uint const& a) noexcept
    {
        // Do not allow any endian transformations on this memory
-        h(a.pn, sizeof(a.pn));
+        h(a.pn.data (), sizeof(a.pn));


Given:

std::array<T, N> a; T b[N];

are we guaranteed that sizeof(a) == sizeof(b)?

I believe so. According to CppRef, "This container is an aggregate type with the same semantics as a struct holding a C-style array T[N] as its only non-static data member. "

Yes, I'm pretty sure that's the case. In the C++17 standard Section 26.3.7.1, Class template array overview paragraph 1 says in part:

An instance of array<T, N> stores N elements of type T, so that size() == N is an invariant.

Then paragraph 2 says:

An array is an aggregate (11.6.1) that can be list-initialized with up to N elements whose
types are convertible to T.

Between those two guarantees I think we're good.

No, especially when N == 0. That being said, the following program compiles on all of our target platforms:

#include <array> #include <cstdint> template <class T, std::size_t N> void test() { std::array<T, N> a; T b[N]; static_assert(sizeof(a) == sizeof(b), ""); } int main() { test<char, 1>(); test<char, 2>(); test<char, 3>(); test<short, 1>(); test<short, 2>(); test<short, 3>(); test<int, 1>(); test<int, 2>(); test<int, 3>(); test<long long, 1>(); test<long long, 2>(); test<long long, 3>(); }

It seems that in practice on all major vendors, this holds for all N other than N == 0 like @HowardHinnant alluded to. See this StackOverflow post for some more standardese-explanation.

Yipes! I missed zero. Good point.

Yeah, I went down this path thinking about the "degenerate" case where N == 0 and then wondered if there are other cases where this could happen.

Do people have a preference of how to defend against this? I am leaning towards just adding a static_assert(WIDTH != 0, "Cannot have 0-sized base_uint"); to avoid these sort of surprises.

Great observation about the N == 0 case @nbougalis.

@JoeLoser base_uint already has

static_assert (Bits >= 64, "The length of a base_uint in bits must be at least 64.");

Also, @HowardHinnant suggestion of static_assert(sizeof(pn) == bytes, "") will also protect against this. I don't think we need a WIDTH != 0 check

JoeLoser · 2018-05-09T21:27:55Z

src/ripple/basics/base_uint.h

@@ -143,7 +143,7 @@ class base_uint
        assert (vch.size () == size ());

        if (vch.size () == size ())


Since we already assert this condition right above, I'm just going to collapse this conditional in my patch later today.

I wouldn't remove the condition. Although it would be a logic error is the sizes don't match, it's also a change in behavior that would fall outside the scope of this PR. We do sometimes do small unrelated cleanups in PRs, but base_uint is a pretty fundamental type and changes in behavior need to be carefully considered.

So good idea, but my vote is "not for this PR".

Very valid points. I'll save it for a different PR. Unnecessary coupling isn't good, especially given how new I am to the codebase.

JoeLoser · 2018-05-13T02:18:03Z

@seelabs I believe I've addressed all the great feedback. I opted to punt on Nik's comment and created a GH issue to track that idea.

I didn't know how you would feel about me pushing directly to seelabs:joe-std-array to kick the Jenkins build until @mellery451 Jenkins changes land. So, here are the commits to rebase onto your remote branch to update this PR.

seelabs · 2018-05-14T12:32:30Z

Pushed latest changes from @JoeLoser

HowardHinnant

Looks good to me. Needs rebase though.

JoeLoser · 2018-05-30T22:59:01Z

I rebased my original branch here, but I'll need @seelabs to rebase the joe-std-array branch.

seelabs · 2018-05-31T16:01:34Z

Rebased and squashed

seelabs · 2018-06-01T21:11:13Z

This should have been marked as passed and put into 1.1.0-b2. Apologies @JoeLoser, we'll get it into the next beta.

JoeLoser · 2018-06-01T22:47:10Z

@seelabs no worries. Next beta is in a week or so I imagine, so no big deal.

mellery451 · 2018-06-20T01:04:14Z

in 0b2f33d

seelabs mentioned this pull request May 9, 2018

Prefer std::array over C-style array #2509

Closed

seelabs requested a review from HowardHinnant May 9, 2018 12:58

seelabs assigned HowardHinnant May 9, 2018

HowardHinnant reviewed May 9, 2018

View reviewed changes

nbougalis reviewed May 9, 2018

View reviewed changes

JoeLoser reviewed May 9, 2018

View reviewed changes

JoeLoser mentioned this pull request May 13, 2018

Revisit need for compare in base_uint.h #2525

Closed

seelabs force-pushed the joe-std-array branch from d80d74a to 82f4029 Compare May 14, 2018 12:31

HowardHinnant approved these changes May 30, 2018

View reviewed changes

HowardHinnant added the Rebase label May 30, 2018

Prefer std::array over C-style array in base_uint

7cb4972

seelabs force-pushed the joe-std-array branch from 82f4029 to 7cb4972 Compare May 31, 2018 16:01

seelabs added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. and removed Rebase labels May 31, 2018

mellery451 closed this Jun 20, 2018

seelabs deleted the joe-std-array branch July 11, 2018 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use std::array in base_uint instead of C-style array #2514

Use std::array in base_uint instead of C-style array #2514

seelabs commented May 9, 2018

seelabs commented May 9, 2018

ripplelabs-jenkins commented May 9, 2018 •

edited

Loading

codecov-io commented May 9, 2018

HowardHinnant May 9, 2018

nbougalis May 9, 2018

JoeLoser May 11, 2018

JoeLoser May 13, 2018 •

edited

Loading

HowardHinnant commented May 9, 2018

JoeLoser commented May 9, 2018

HowardHinnant commented May 9, 2018

nbougalis May 9, 2018 •

edited

Loading

JoeLoser May 9, 2018

scottschurr May 9, 2018

HowardHinnant May 9, 2018

JoeLoser May 9, 2018

scottschurr May 9, 2018

nbougalis May 9, 2018

JoeLoser May 11, 2018

seelabs May 11, 2018 •

edited

Loading

JoeLoser May 9, 2018

seelabs May 11, 2018

JoeLoser May 11, 2018 •

edited

Loading

JoeLoser commented May 13, 2018

seelabs commented May 14, 2018

HowardHinnant left a comment

JoeLoser commented May 30, 2018

seelabs commented May 31, 2018

seelabs commented Jun 1, 2018

JoeLoser commented Jun 1, 2018

mellery451 commented Jun 20, 2018

		@@ -143,7 +143,7 @@ class base_uint
		assert (vch.size () == size ());

		if (vch.size () == size ())

Use std::array in base_uint instead of C-style array #2514

Use std::array in base_uint instead of C-style array #2514

Conversation

seelabs commented May 9, 2018

seelabs commented May 9, 2018

ripplelabs-jenkins commented May 9, 2018 • edited Loading

Jenkins Build Summary

Test Results

codecov-io commented May 9, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLoser May 13, 2018 • edited Loading

Choose a reason for hiding this comment

HowardHinnant commented May 9, 2018

JoeLoser commented May 9, 2018

HowardHinnant commented May 9, 2018

nbougalis May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLoser May 11, 2018 • edited Loading

Choose a reason for hiding this comment

JoeLoser commented May 13, 2018

seelabs commented May 14, 2018

HowardHinnant left a comment

Choose a reason for hiding this comment

JoeLoser commented May 30, 2018

seelabs commented May 31, 2018

seelabs commented Jun 1, 2018

JoeLoser commented Jun 1, 2018

mellery451 commented Jun 20, 2018

ripplelabs-jenkins commented May 9, 2018 •

edited

Loading

JoeLoser May 13, 2018 •

edited

Loading

nbougalis May 9, 2018 •

edited

Loading

seelabs May 11, 2018 •

edited

Loading

JoeLoser May 11, 2018 •

edited

Loading