Switch Implementation #43

wusatosi · 2024-11-28T17:48:41Z

This PR switches current implementation to reference implementation in the paper by @gnzlbg .

Original link: https://godbolt.org/z/5P78aG5xE

Permission to switch this implementation was granted by @camio .

Fixed CI and included test suite from original implementation.

Let's not scrutinize the details of the implementation yet.

CC: @jbab

Fix #39

tests/beman/inplace_vector/ref_impl.test.cpp

.github/workflows/ci_tests.yml

DeveloperPaul123

Seems fine to me. I don't have the time to fully review the ref impl, but I assume it's fine.

wusatosi · 2024-12-03T16:57:46Z

Seems fine to me. I don't have the time to fully review the ref impl, but I assume it's fine.

We will do that later. This is just switching to reference implementation

ClausKlein · 2024-12-03T18:11:41Z

README.md

 ### Compiler support

-Building this repository requires **C++17** or later.
+Building this repository requires **C++23** or later.


change below, line 73, too

Agh, missed this.

ClausKlein · 2024-12-03T18:18:35Z

This code need some more love:

bash-5.2$ run-clang-tidy -p build -checks='-*,bugprone-*'
Enabled checks:
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-assignment-in-if-condition
    bugprone-bad-signal-to-kill-thread
    bugprone-bool-pointer-implicit-conversion
    bugprone-branch-clone
    bugprone-casting-through-void
    bugprone-chained-comparison
    bugprone-compare-pointer-to-member-virtual-function
    bugprone-copy-constructor-init
    bugprone-crtp-constructor-accessibility
    bugprone-dangling-handle
    bugprone-dynamic-static-initializers
    bugprone-easily-swappable-parameters
    bugprone-empty-catch
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
    bugprone-forwarding-reference-overload
    bugprone-implicit-widening-of-multiplication-result
    bugprone-inaccurate-erase
    bugprone-inc-dec-in-conditions
    bugprone-incorrect-enable-if
    bugprone-incorrect-roundings
    bugprone-infinite-loop
    bugprone-integer-division
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-pointer-arithmetic-in-alloc
    bugprone-misplaced-widening-cast
    bugprone-move-forwarding-reference
    bugprone-multi-level-implicit-pointer-conversion
    bugprone-multiple-new-in-one-expression
    bugprone-multiple-statement-macro
    bugprone-narrowing-conversions
    bugprone-no-escape
    bugprone-non-zero-enum-to-bool-conversion
    bugprone-not-null-terminated-result
    bugprone-optional-value-conversion
    bugprone-parent-virtual-call
    bugprone-pointer-arithmetic-on-polymorphic-object
    bugprone-posix-return
    bugprone-redundant-branch-condition
    bugprone-reserved-identifier
    bugprone-return-const-ref-from-parameter
    bugprone-shared-ptr-array-mismatch
    bugprone-signal-handler
    bugprone-signed-char-misuse
    bugprone-sizeof-container
    bugprone-sizeof-expression
    bugprone-spuriously-wake-up-functions
    bugprone-standalone-empty
    bugprone-string-constructor
    bugprone-string-integer-assignment
    bugprone-string-literal-with-embedded-nul
    bugprone-stringview-nullptr
    bugprone-suspicious-enum-usage
    bugprone-suspicious-include
    bugprone-suspicious-memory-comparison
    bugprone-suspicious-memset-usage
    bugprone-suspicious-missing-comma
    bugprone-suspicious-realloc-usage
    bugprone-suspicious-semicolon
    bugprone-suspicious-string-compare
    bugprone-suspicious-stringview-data-usage
    bugprone-swapped-arguments
    bugprone-switch-missing-default-case
    bugprone-terminating-continue
    bugprone-throw-keyword-missing
    bugprone-too-small-loop-variable
    bugprone-unchecked-optional-access
    bugprone-undefined-memory-manipulation
    bugprone-undelegated-constructor
    bugprone-unhandled-exception-at-new
    bugprone-unhandled-self-assignment
    bugprone-unique-ptr-array-mismatch
    bugprone-unsafe-functions
    bugprone-unused-local-non-trivial-variable
    bugprone-unused-raii
    bugprone-unused-return-value
    bugprone-use-after-move
    bugprone-virtual-near-miss

Running clang-tidy for 3 files out of 3 in compilation database ...
[1/3][2.9s] /usr/local/opt/llvm/bin/clang-tidy -checks=-*,bugprone-* -p=build /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp:75:7: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
   75 |     } catch (const std::out_of_range &) {
      |       ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp:83:7: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
   83 |     } catch (const std::out_of_range &) {
      |       ^
27827 warnings generated.
Suppressed 27825 warnings (27825 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

[2/3][3.6s] /usr/local/opt/llvm/bin/clang-tidy -checks=-*,bugprone-* -p=build /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/examples/fibonacci.cpp
33173 warnings generated.
Suppressed 33173 warnings (33173 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

[3/3][4.7s] /usr/local/opt/llvm/bin/clang-tidy -checks=-*,bugprone-* -p=build /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:16: warning: 2 adjacent parameters of 'non_copyable' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                ^~~~~~~~~~~~~~~
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:20: note: the first parameter in the range is 'i'
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                    ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:30: note: the last parameter in the range is 'd'
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                              ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:23: note: 'int' and 'double' may be implicitly converted
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                       ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:336:5: warning: an exception may be thrown in function 'main' which should not throw exceptions [bugprone-exception-escape]
  336 | int main() {
      |     ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:338:42: warning: declaration uses identifier '__non_trivial', which is a reserved identifier [bugprone-reserved-identifier]
  338 |     using beman::__iv_detail::__storage::__non_trivial;
      |                                          ^~~~~~~~~~~~~
      |                                          _non_trivial
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:339:42: warning: declaration uses identifier '__trivial', which is a reserved identifier [bugprone-reserved-identifier]
  339 |     using beman::__iv_detail::__storage::__trivial;
      |                                          ^~~~~~~~~
      |                                          _trivial
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:340:42: warning: declaration uses identifier '__zero_sized', which is a reserved identifier [bugprone-reserved-identifier]
  340 |     using beman::__iv_detail::__storage::__zero_sized;
      |                                          ^~~~~~~~~~~~
      |                                          _zero_sized
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:486:11: warning: 'a' used after it was moved [bugprone-use-after-move]
  486 |     CHECK(a.size() == std::size_t{3});
      |           ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:484:7: note: move occurred here
  484 |     b = std::move(a);
      |       ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:495:11: warning: 'a' used after it was moved [bugprone-use-after-move]
  495 |     CHECK(a.size() == std::size_t{3});
      |           ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:493:25: note: move occurred here
  493 |     vector<MoveOnly, 3> b(std::move(a));
      |                         ^
33493 warnings generated.
Suppressed 33486 warnings (33486 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

bash-5.2$

ClausKlein · 2024-12-03T18:28:57Z

Try this too!

cmake -S . -B build -G Ninja -DCMAKE_CXX_STANDARD=23 -DCMAKE_EXPORT_COMPILE_COMMANDS=ON --fresh
# ...
run-clang-tidy -p build -checks='-*,cppcoreguidelines-pro-*'
Enabled checks:
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg

...

wusatosi · 2024-12-06T03:07:20Z

Hey @ClausKlein sorry I didn't reply to your message before merging.

This is simply to port the reference implementation. We might incorporate suggestions from clang-tidy but the main concern currently is to review the reference implementation against the paper. Plus clang tidy gives pedantic warnings that's often of no value.

I don't really think we should do clang-tidy check for test files, especially when this clearly checks exception throwing:

[1/3][2.9s] /usr/local/opt/llvm/bin/clang-tidy -checks=-*,bugprone-* -p=build /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp:75:7: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
   75 |     } catch (const std::out_of_range &) {
      |       ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp:83:7: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
   83 |     } catch (const std::out_of_range &) {
      |       ^
27827 warnings generated.
Suppressed 27825 warnings (27825 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

I don't think this is valuable for testing structs, also there's no instance of misuse reported.

[3/3][4.7s] /usr/local/opt/llvm/bin/clang-tidy -checks=-*,bugprone-* -p=build /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:16: warning: 2 adjacent parameters of 'non_copyable' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                ^~~~~~~~~~~~~~~
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:20: note: the first parameter in the range is 'i'
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                    ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:30: note: the last parameter in the range is 'd'
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                              ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:139:23: note: 'int' and 'double' may be implicitly converted
  139 |   non_copyable(int i, double d) : i_(i), d_(d) {}
      |                       ^

Why not throw exception for tests at main? I don't think try-catch the main function and return -1 is of any value for tests.

/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:336:5: warning: an exception may be thrown in function 'main' which should not throw exceptions [bugprone-exception-escape]
  336 | int main() {
      |     ^

Reference implementation aimed at std-style implementation (originally under std namespace). We would like to move away from reserved identifier but it's not top priority.

/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:338:42: warning: declaration uses identifier '__non_trivial', which is a reserved identifier [bugprone-reserved-identifier]
  338 |     using beman::__iv_detail::__storage::__non_trivial;
      |                                          ^~~~~~~~~~~~~
      |                                          _non_trivial
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:339:42: warning: declaration uses identifier '__trivial', which is a reserved identifier [bugprone-reserved-identifier]
  339 |     using beman::__iv_detail::__storage::__trivial;
      |                                          ^~~~~~~~~
      |                                          _trivial
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:340:42: warning: declaration uses identifier '__zero_sized', which is a reserved identifier [bugprone-reserved-identifier]
  340 |     using beman::__iv_detail::__storage::__zero_sized;
      |                                          ^~~~~~~~~~~~
      |                                          _zero_sized

The intent is literally to test the move-from object:

/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:486:11: warning: 'a' used after it was moved [bugprone-use-after-move]
  486 |     CHECK(a.size() == std::size_t{3});
      |           ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:484:7: note: move occurred here
  484 |     b = std::move(a);
      |       ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:495:11: warning: 'a' used after it was moved [bugprone-use-after-move]
  495 |     CHECK(a.size() == std::size_t{3});
      |           ^
/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/ref_impl.test.cpp:493:25: note: move occurred here
  493 |     vector<MoveOnly, 3> b(std::move(a));
      |                         ^

I wish this line by line explanation shows why I am not a big advocate for clang-tidy, I am sorry but this report is not helpful at all to our implementation.

wusatosi added 10 commits November 28, 2024 12:16

use reference impl by Gonzalo Brito Gadeschi

0ce93f2

update cpp version

0dc15bb

swap namespace

d79d706

update CI to use latest compiler

6884eb7

update CI

dd96217

disable __IV_EXPECT

bad3312

fix from_range_t

7597a3a

fix ci

643dd4c

surpress unused parameter

1e83a61

delete unneeded comment

2ccc335

wusatosi requested review from DeveloperPaul123, Hels15 and camio as code owners November 28, 2024 17:48

migrate test

1a9e545

wusatosi force-pushed the ref-impl branch from b74a4c3 to 1a9e545 Compare November 28, 2024 17:50

lint

0ed5301

wusatosi force-pushed the ref-impl branch from 5e47127 to 0ed5301 Compare November 28, 2024 17:53

trying to play well with werror

92dc268

github-actions bot reviewed Nov 28, 2024

View reviewed changes

tests/beman/inplace_vector/ref_impl.test.cpp Outdated Show resolved Hide resolved

tests/beman/inplace_vector/ref_impl.test.cpp Outdated Show resolved Hide resolved

wusatosi added 2 commits November 28, 2024 13:03

format

657034a

update README

40d3378

DeveloperPaul123 reviewed Dec 3, 2024

View reviewed changes

.github/workflows/ci_tests.yml Show resolved Hide resolved

DeveloperPaul123 approved these changes Dec 3, 2024

View reviewed changes

ClausKlein reviewed Dec 3, 2024

View reviewed changes

wusatosi merged commit 2b0bb62 into bemanproject:main Dec 3, 2024
11 checks passed

wusatosi deleted the ref-impl branch December 3, 2024 22:34

wusatosi mentioned this pull request Dec 4, 2024

Enhanced unit tests from original reference implementation #4

Closed

wusatosi mentioned this pull request Jun 2, 2025

How to use clang-tidy? bemanproject/exemplar#174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch Implementation #43

Switch Implementation #43

Uh oh!

wusatosi commented Nov 28, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeveloperPaul123 left a comment

Uh oh!

wusatosi commented Dec 3, 2024

Uh oh!

ClausKlein Dec 3, 2024

Uh oh!

wusatosi Dec 6, 2024

Uh oh!

ClausKlein commented Dec 3, 2024

Uh oh!

ClausKlein commented Dec 3, 2024

Uh oh!

Uh oh!

wusatosi commented Dec 6, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Switch Implementation #43

Switch Implementation #43

Uh oh!

Conversation

wusatosi commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeveloperPaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

wusatosi commented Dec 3, 2024

Uh oh!

ClausKlein Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

wusatosi Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

ClausKlein commented Dec 3, 2024

Uh oh!

ClausKlein commented Dec 3, 2024

Try this too!

Uh oh!

Uh oh!

wusatosi commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wusatosi commented Nov 28, 2024 •

edited

Loading

wusatosi commented Dec 6, 2024 •

edited

Loading