Fix format + Make project buildable #3

wusatosi · 2025-02-06T04:16:31Z

This PR fixes current CI failure and makes the project buildable.

robert-andrzejuk

@wusatosi Thank you for fixing this.
It brings such pleasure to the eyes - to see more green than red.

tests/beman/scope/scope.test.cpp

JeffGarland · 2025-02-06T13:12:42Z

README.md

 # Overview

-During the C++20 cycle [P0052 Generic Scope Guard and RAII Wrapper for the Standard Library](https://wg21.link/P0052) added 4 types: `scope_exit`, `scope_fail`, `scope_success` and `unique_resource` to [LTFSv3](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/n4908#scopeguard).  In the intervening time, two standard libraries have implemented support as well as Boost. With the imperative for safety and security in C++ developers need every tool in the toolbox. The authors believe it is time to move this facility into the standard.  The paper will re-examine the five year old design and any learning from deployment of the LTFSv3.
+During the C++20 cycle [P0052 Generic Scope Guard and RAII Wrapper for the Standard Library](https://wg21.link/P0052)


As I expressed in discourse I believe this change should be reverted and the linter line length restriction removed.

I'm approving, and if this isn't reverted when you merge I'll fix it in the next PR :)

JeffGarland · 2025-02-06T13:18:06Z

include/beman/scope/scope.hpp


-namespace beman::scope {} // namespace beman::scope
+namespace beman::scope {
+


This now makes the example not runnable -- before it was ok on 2 major compilers. Please see this post for a path forward -https://discourse.bemanproject.org/t/scope-library/315/14?u=jeff-garland

If BEMAN_USE_SCOPE_TS3 is set experimental will be included and beman::scope will just be using's in for experimental. For now BEMAN_USE_SCOPE_TS3 should default to true. This allows us to write tests and examples with no further development.

I checked your post linked, I don't quite understand what you mean here.

I believe you are referencing:

For moving forward, I think as I mentioned elsewhere we should leverage the current implementations – and maintain those going forward. So we would basically have a macro like BEMAN_USE_SCOPE_TS3 that would compile things based on the native implementation. So rev 0 of the implementation would amount to including experimental and some using statements. This allows us to setup tests and examples while considering changes to the design and perhaps re-implementing some things.

Are we providing an implementation here? Or are we just compare-contrasting current implementations?

Or are you proposing we do:

namespace beman::scope { using scope_exit = std::experimental::scope_exit; }

And in future PRs, expand it to:

namespace beman::scope { #if BEMAN_USE_SCOPE_TS3 using scope_exit = std::experimental::scope_exit; #elif BEMAN_USE_SCOPE_BOOST using scope_exit = boost::scope::scope_exit; #endif }

What do you mean by not runnable here, do you mean we are unable to compile it, or do you mean it like segfault or do you mean its behavior is incorrect? Current implementation is a no-op TODO so that the examples compiles with upcoming beman implementations.

Note the scope of this PR is just so the project passes CI and is build-able. I am not intending to provide a useable implementation here. We should implement BEMAN_USE_SCOPE_TS3 in another PR.

Yes, the proposal is as you said.

It's a fair point, however, about not providing a usable implementation in this PR -- fine with that, but I think it'll be pretty trivial to do and will keep the examples/tests actually running code instead of a stub. Aim high :) Effectively 2 #includes and 4 using statements -- noting that the example that was there was buildable and runnable. As it stands it's only buildable.

As for providing our own implementation here, I expect that will eventually happen -- but at first I'm more interested in developing tests/examples the push the TS implementations and explore the design space. It's my educated guess that no users know about the TS implementations and so there's whatever tests the developers wrote and not much real world usage.

Alright, I didn't know what you were expecting here, I was not comfortable implementing this whole thing and I wasn't aware you were only looking for an alias.

I can implement the switch in another PR.

I have implemented a "rough" version, to make the example work:
https://gist.github.com/robert-andrzejuk/ad07ddcde22d3ffbce167e57e8530889

This needs lots of work, but it makes the example work.
Should I add it?

hey @robert-andrzejuk , thanks for the gist.

Let's do this in another PR. I want to keep the scope managable. I will just do the using alias.
Feel free to open a PR for your implementation.

@JeffGarland I think we should move forward with this PR as is.

<experimental/scope> is not a standard header and setting it up is not trivial as it requires special build arguments and selective compiler support.

This needs non-trivial work to our infrastructure and docs. It does not make sense to update them in this PR.

That's fine -- I'd like to move past this PR -- but I think you're overthinking this :)

JeffGarland · 2025-02-06T13:19:45Z

CMakeLists.txt

-#    add_subdirectory(tests/beman/scope)
-#endif()
+if(BEMAN_SCOPE_BUILD_TESTS)
+    add_subdirectory(tests/beman/scope)


Didn't we decide to flatten these directories to TOP/tests and TOP/src -- if we didn't we should. Only for headers do we need the distinction. And yes, I wouldn't be surprised if exemplar if wrong

No, it is not flattened, exemplar is correct here. This is codified in the beman standard, see here.

[DIRECTORY.SOURCES] RECOMMENDATION: Sources and headers not part of the public interface should reside in the top-level src/ directory, and should use the same structure from include/ - e.g., src/beman/<short_name>/. Check CMAKE.AVOID_PASSTHROUGHS.

[DIRECTORY.TESTS] REQUIREMENT: All test files must reside within the top-level tests/ directory, and should use the same structure from include/. If multiple test types are present, subdirectories can be made (e.g., unit tests, performance etc).

I believe this is so that we can do a giant beman build, basically merging all beman projects together and check if they can compile together as a single library.

We decided to not flatten examples directory.

Boost does a giant build and doesn't require this -- we should rediscuss this point.

Yeah the directory structure is not negotiable here. You will have to change the beman standard.

Feel free to bring it up at the next sync.

Note that it might be a little bit too late to change this, all current projects follow this folder structure. You will have to update a lot of current projects if you want to change this rule.

It's never too late to change things

Co-authored-by: Robert Andrzejuk <[email protected]>

examples/scope_example.cpp

examples/unique_resource_example.cpp

include/beman/scope/scope.hpp

wusatosi added 6 commits February 5, 2025 21:35

fix readme

c534433

make configurable

a463033

fix tests

5650c6b

format

d705cdc

make project buildable

2c64e3e

add todo destructors

e419df0

wusatosi requested review from bretbrownjr, camio, dietmarkuehl, neatudarius and steve-downey as code owners February 6, 2025 04:16

robert-andrzejuk reviewed Feb 6, 2025

View reviewed changes

tests/beman/scope/scope.test.cpp Show resolved Hide resolved

robert-andrzejuk requested review from JeffGarland and robert-andrzejuk and removed request for bretbrownjr, camio, dietmarkuehl, neatudarius and steve-downey February 6, 2025 12:29

JeffGarland reviewed Feb 6, 2025

View reviewed changes

Update tests/beman/scope/scope.test.cpp

ca7d280

Co-authored-by: Robert Andrzejuk <[email protected]>

wusatosi commented Feb 6, 2025

View reviewed changes

examples/scope_example.cpp Show resolved Hide resolved

Robert-Andrzejuk-Infor added 2 commits February 7, 2025 22:11

Copy synopsis from p0052r10.pdf

1a55b42

Empty example file for unique_resource example code

7208ca9

github-actions bot reviewed Feb 7, 2025

View reviewed changes

examples/unique_resource_example.cpp Outdated Show resolved Hide resolved

change order of synopsis and code, remove merge markings

cc8f4bd

github-actions bot reviewed Feb 7, 2025

View reviewed changes

include/beman/scope/scope.hpp Outdated Show resolved Hide resolved

JeffGarland approved these changes Feb 7, 2025

View reviewed changes

wusatosi added 2 commits February 7, 2025 16:50

format

7bf9fb0

Merge branch 'main' into fix-ci

478754e

wusatosi mentioned this pull request Feb 7, 2025

CI runs twice when commit local branch is in a pull request bemanproject/exemplar#116

Closed

wusatosi merged commit 37abed8 into main Feb 7, 2025
128 checks passed

wusatosi deleted the fix-ci branch February 7, 2025 22:06

wusatosi mentioned this pull request Feb 8, 2025

Delegate implementation to std::experimental #12

Merged

1 task


		namespace beman::scope {} // namespace beman::scope
		namespace beman::scope {

Fix format + Make project buildable #3

Fix format + Make project buildable #3

Uh oh!

Conversation

wusatosi commented Feb 6, 2025

Uh oh!

robert-andrzejuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffGarland Feb 7, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffGarland Feb 6, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusatosi Feb 7, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JeffGarland Feb 7, 2025 •

edited

Loading

JeffGarland Feb 6, 2025 •

edited

Loading

wusatosi Feb 7, 2025 •

edited

Loading