Changed pragma once to include guard, Added maybe_unused to unused params, Removed comment cruft #75

Intedai · 2025-05-08T13:05:20Z

River (@wusatosi ):
fixes #79
fixes #73

…rams, Removed comment cruft

Intedai · 2025-05-08T13:06:36Z

Issue: #73

JeffGarland

This looks good to me.

~~Of course I'm realizing this repo basically doesn't have any testing turned on! ugh! New issue incoming...~~

I see you have to click approve and run workflows for the CI to run.

edit: Forgot to say welcome @Intedai - thank you for your contribution!

Intedai · 2025-05-08T14:05:11Z

This looks good to me.

~~Of course I'm realizing this repo basically doesn't have any testing turned on! ugh! New issue incoming...~~

I see you have to click approve and run workflows for the CI to run.

edit: Forgot to say welcome @Intedai - thank you for your contribution!

Thanks! 😃

JeffGarland · 2025-05-08T14:48:58Z

Please merge when ready.

DeveloperPaul123

Looks good to me! Nice work 👍

wusatosi · 2025-05-08T16:22:15Z

include/beman/inplace_vector/inplace_vector.hpp

 private: // Utilities
-  constexpr void assert_iterator_in_range(const_iterator it) noexcept {
+  constexpr void
+  assert_iterator_in_range([[maybe_unused]] const_iterator it) noexcept {


These functions are left over artifacts from the original reference implementation. iirc they're not used anywhere, IV_EXPECT is defined as a no-op currently, I think it's safe to remove all these artifacts

wusatosi · 2025-05-08T16:23:49Z

include/beman/inplace_vector/inplace_vector.hpp

+  static constexpr void
+  unsafe_set_size([[maybe_unused]] size_t new_size) noexcept {
    IV_EXPECT(new_size == 0 &&
              "tried to change size of empty storage to non-zero value");
  }


Suggested change

static constexpr void

unsafe_set_size([[maybe_unused]] size_t new_size) noexcept {

IV_EXPECT(new_size == 0 &&

"tried to change size of empty storage to non-zero value");

}

static constexpr void

unsafe_set_size(size_t) noexcept {}

Does this work?
This is assuming we remove all the assertion related artifact.

wusatosi

Thanks for the help, we can remove the artifacts from the assertion factilities in another PR.

JeffGarland · 2025-05-08T18:10:03Z

Thanks for the help, we can remove the artifacts from the assertion factilities in another PR.

we should make an issue for this work. I think, without looking at the original, the intent of these are to be static asserts for preconditions which could be enabled. So if we agree that this is the case we should consider seeing if we can get a compiler with contracts and turn them into contract preconditions -- and maybe otherwise they just turn into asserts.

See this paper https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3471r4.html for details. Just to extract one thing:

As an example:

a.front()
Modify 23.2.4 [sequence.reqmts] around paragraph 69 as indicated:
a.front()
69 Result: reference; const_reference for constant a.
70 Hardened preconditions: a.empty() is false.
71 Returns: *a.begin()
72 Remarks: Required for basic_string, array, deque, forward_list, inplace_vector, list, and vector.

20162026

there is some more comment cruft left at:

inplace_vector/include/beman/inplace_vector/inplace_vector.hpp

Line 505 in c803c83

// constexpr void resize(size_type sz);

inplace_vector/include/beman/inplace_vector/inplace_vector.hpp

Line 520 in c803c83

// constexpr const_reference at(size_type n) const;

inplace_vector/include/beman/inplace_vector/inplace_vector.hpp

Line 543 in c803c83

// constexpr friend auto /*synth-three-way-result<T>*/

wusatosi · 2025-05-08T21:09:02Z

we should make an issue for this work. I think, without looking at the original, the intent of these are to be static asserts for preconditions which could be enabled. So if we agree that this is the case we should consider seeing if we can get a compiler with contracts and turn them into contract preconditions -- and maybe otherwise they just turn into asserts.

As far as I know there hasn't been any compiler releases that support contracts. Let's hold this off till there's some compiler support as we currently do not have infrastructures that builds custom compilers for POCs.

Can you open an issue about this? For now I think we should simply remove all the artifact. We can do this in another PR or in the same PR.

JeffGarland · 2025-05-09T15:47:11Z

we should make an issue for this work. I think, without looking at the original, the intent of these are to be static asserts for preconditions which could be enabled. So if we agree that this is the case we should consider seeing if we can get a compiler with contracts and turn them into contract preconditions -- and maybe otherwise they just turn into asserts.

As far as I know there hasn't been any compiler releases that support contracts.

If you look on godbolt at least gcc has several experimental versions with contracts -- I'm not sure which one (if any) corresponds to P2900.

Let's hold this off till there's some compiler support as we currently do not have infrastructures that builds custom compilers for POCs.

Again, there's nothing stopping someone from making those plain old assert checks -- contracts is just a fancier way to do that and have more control over the runtime results.

Can you open an issue about this? For now I think we should simply remove all the artifact. We can do this in another PR or in the same PR.

Done: #78

wusatosi · 2025-05-09T16:18:07Z

If you look on godbolt at least gcc has several experimental versions with contracts -- I'm not sure which one (if any) corresponds to P2900.

We have no infrastructure to test on experimental versions of gcc, this is on my road map but it's not there yet. I am still very much against putting things in that are not testable.

Again, there's nothing stopping someone from making those plain old assert checks -- contracts is just a fancier way to do that and have more control over the runtime results.

We can bring back the assertion facilities if that's what you're suggesting.

Thanks for opening the issue.

github-actions · 2025-05-10T17:16:14Z

include/beman/inplace_vector/inplace_vector.hpp


 #undef IV_EXPECT
+
+#endif // BEMAN_INPLACE_VECTOR_INPLACE_VECTOR_HPP


[pre-commit] _{reported by reviewdog 🐶}

Suggested change

#endif // BEMAN_INPLACE_VECTOR_INPLACE_VECTOR_HPP

#endif // BEMAN_INPLACE_VECTOR_INPLACE_VECTOR_HPP

Intedai · 2025-05-10T17:33:25Z

Fixed merge conflict, sorry about the second commit accidentally removed the new line at the end 😢

wusatosi · 2025-05-10T19:05:30Z

Fixed merge conflict, sorry about the second commit accidentally removed the new line at the end 😢

No you're fine, thanks for helping out!

There's a bit more cruft left, see @20162026 's comment.

Otherwise let me know when you think this PR is ready to merge.

Intedai · 2025-05-10T22:36:51Z

@wusatosi I removed the remaining cruft, I'm pretty sure the pr is ready to merge

Changed pragma once to include guard, Added maybe_unused to unused pa…

c803c83

…rams, Removed comment cruft

Intedai requested review from DeveloperPaul123, Hels15, camio and wusatosi as code owners May 8, 2025 13:05

Intedai requested a review from a team May 8, 2025 13:05

JeffGarland approved these changes May 8, 2025

View reviewed changes

DeveloperPaul123 approved these changes May 8, 2025

View reviewed changes

wusatosi reviewed May 8, 2025

View reviewed changes

wusatosi approved these changes May 8, 2025

View reviewed changes

20162026 requested changes May 8, 2025

View reviewed changes

JeffGarland mentioned this pull request May 9, 2025

Change the IV_EXPECT macro into something useful #78

Open

wusatosi mentioned this pull request May 10, 2025

warning C4068: unknown pragma 'GCC' #79

Closed

Merge branch 'main' into cleanup

5b3b7ea

github-actions bot reviewed May 10, 2025

View reviewed changes

Merge conflict solved, now passes pre-commit tests

3c83e91

removed more comment cruft

bc390c1

JeffGarland merged commit b8aabf6 into bemanproject:main May 10, 2025
25 checks passed


		#undef IV_EXPECT

		#endif // BEMAN_INPLACE_VECTOR_INPLACE_VECTOR_HPP

Changed pragma once to include guard, Added maybe_unused to unused params, Removed comment cruft #75

Changed pragma once to include guard, Added maybe_unused to unused params, Removed comment cruft #75

Uh oh!

Conversation

Intedai commented May 8, 2025 • edited by wusatosi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Intedai commented May 8, 2025

Uh oh!

JeffGarland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Intedai commented May 8, 2025

Uh oh!

JeffGarland commented May 8, 2025

Uh oh!

DeveloperPaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

wusatosi May 8, 2025

Choose a reason for hiding this comment

Uh oh!

wusatosi May 8, 2025

Choose a reason for hiding this comment

Uh oh!

wusatosi left a comment

Choose a reason for hiding this comment

Uh oh!

JeffGarland commented May 8, 2025

Uh oh!

20162026 left a comment

Choose a reason for hiding this comment

Uh oh!

wusatosi commented May 8, 2025

Uh oh!

JeffGarland commented May 9, 2025

Uh oh!

wusatosi commented May 9, 2025

Uh oh!

github-actions bot May 10, 2025

Choose a reason for hiding this comment

Uh oh!

Intedai commented May 10, 2025

Uh oh!

wusatosi commented May 10, 2025

Uh oh!

Intedai commented May 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Intedai commented May 8, 2025 •

edited by wusatosi

Loading

JeffGarland left a comment •

edited

Loading