Refactor Stack utility class #495

chfast · 2020-08-18T08:55:13Z

I started this to investigate "containter-overflow" issues reported by ASan in XCode 12.0 beta. This does not fix the issue, but at least I minimized the Stack interface and we don't use inheritance any more.

axic · 2020-08-23T09:59:10Z

@chfast I'm in favour of this, what is blocking here? Is the goal just cleanup or some speed benefit for parsing?

chfast · 2020-08-23T12:40:55Z

Nothing is blocking this, but unit tests must be updated. See updated description.

axic · 2020-09-02T09:59:41Z

I'd prefer this PR, unless there is some significant parser slowdown.

axic · 2020-09-02T14:20:20Z

test/unittests/stack_test.cpp

-    EXPECT_EQ(stack.size(), 0);
-}
-
-TEST(stack, resize)


There's no test for shrink besides this.

axic · 2020-09-02T14:21:22Z

Could rename the drop_and_peek test to drop. And maybe group the struct_item test with the others.

lib/fizzy/stack.hpp

codecov · 2020-09-02T14:42:58Z

Codecov Report

Merging #495 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   99.69%   99.69%   -0.01%     
==========================================
  Files          54       54              
  Lines       17180    17091      -89     
==========================================
- Hits        17128    17039      -89     
  Misses         52       52

axic · 2020-09-02T16:40:27Z

lib/fizzy/stack.hpp

    }
+
+    /// Drops @a num_elements elements from the top of the stack.
+    void drop(size_t num_elements = 1) noexcept { shrink(size() - num_elements); }


Hmm, shouldn't this need assert(size() >= num_elements), since size_t is unsigned, the value to shrink could wrap around?

OperandStack::drop has assert(num_elements <= size()) so maybe follow that style.

I removed drop().

axic · 2020-09-02T16:43:48Z

The rest looks good to me.

Does this result in any significant regression in parsing?

chfast · 2020-09-02T20:49:57Z

Not really a change performance-wise

fizzy/parse/blake2b_mean                                -0.0114         -0.0114            23            23            23            23
fizzy/parse/ecpairing_mean                              -0.0003         -0.0003          1338          1337          1338          1337
fizzy/parse/keccak256_mean                              -0.0095         -0.0094            42            42            42            42
fizzy/parse/memset_mean                                 -0.0034         -0.0034             6             6             6             6
fizzy/parse/mul256_opt0_mean                            -0.0269         -0.0269             8             8             8             8
fizzy/parse/ramanujan_pi_mean                           +0.0160         +0.0160            24            24            24            24
fizzy/parse/sha1_mean                                   -0.0008         -0.0008            38            38            38            38
fizzy/parse/sha256_mean                                 +0.0031         +0.0031            63            63            63            63
fizzy/parse/taylor_pi_mean                              -0.0262         -0.0262             3             3             3             3
fizzy/parse/micro/eli_interpreter_mean                  -0.0167         -0.0167             4             4             4             4
fizzy/parse/micro/factorial_mean                        -0.0392         -0.0392             1             1             1             1
fizzy/parse/micro/fibonacci_mean                        -0.0473         -0.0473             1             1             1             1
fizzy/parse/micro/host_adler32_mean                     -0.0250         -0.0250             2             2             2             2
fizzy/parse/micro/spinner_mean                          -0.0160         -0.0160             1             1             1             1

axic · 2020-09-02T21:30:05Z

lib/fizzy/stack.hpp

    }

-    T pop()
+    T pop() noexcept


Is this marked noexcept because we manually ensure the caller will call it in a bad state? (Enforced by the assert in debug mode) Note that back and pop_back are not noexcept.

Assert is not relevant. This is noexcept because .back() and .pop_back() are noexcept too.

I checked it when I commented on empty/size, but to me back/pop_back did not seem to be noexcept:

http://cplusplus.com/reference/vector/vector/back/

http://cplusplus.com/reference/vector/vector/pop_back/

Ah okay noexcept since C++20:

https://en.cppreference.com/w/cpp/container/vector/back

https://en.cppreference.com/w/cpp/container/vector/pop_back

Were these behaving as noexcept pre-C++20 just not marked so?

Officially they are not noexcept (even in C++20), but in practice they are.

This looks good to me.

axic

Looks okay to me, but would prefer a second look from @gumb0 on the noexcept question.

chfast force-pushed the stack_utils branch 2 times, most recently from 117c18b to ed42f15 Compare September 2, 2020 14:18