Skip to content

Fix format + Make project buildable #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wusatosi merged 12 commits into from
Feb 7, 2025
Merged

Fix format + Make project buildable #3

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c534433
fix readme
wusatosi Feb 6, 2025
a463033
make configurable
wusatosi Feb 6, 2025
5650c6b
fix tests
wusatosi Feb 6, 2025
d705cdc
format
wusatosi Feb 6, 2025
2c64e3e
make project buildable
wusatosi Feb 6, 2025
e419df0
add todo destructors
wusatosi Feb 6, 2025
ca7d280
Update tests/beman/scope/scope.test.cpp
wusatosi Feb 6, 2025
1a55b42
Copy synopsis from p0052r10.pdf
Robert-Andrzejuk-Infor Feb 7, 2025
7208ca9
Empty example file for unique_resource example code
Robert-Andrzejuk-Infor Feb 7, 2025
cc8f4bd
change order of synopsis and code, remove merge markings
Robert-Andrzejuk-Infor Feb 7, 2025
7bf9fb0
format
wusatosi Feb 7, 2025
478754e
Merge branch 'main' into fix-ci
wusatosi Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ cmake_minimum_required(VERSION 3.25)

project(beman.scope DESCRIPTION "Generic Scope Guard" LANGUAGES CXX)

# enable_testing()
enable_testing()

# [CMAKE.SKIP_TESTS]
option(
Expand All @@ -20,14 +20,13 @@ option(
${PROJECT_IS_TOP_LEVEL}
)

include(FetchContent)
include(GNUInstallDirs)

# add_subdirectory(src/beman/scope)
add_subdirectory(src/beman/scope)

#if(BEMAN_SCOPE_BUILD_TESTS)
# add_subdirectory(tests/beman/scope)
#endif()
if(BEMAN_SCOPE_BUILD_TESTS)
add_subdirectory(tests/beman/scope)
Copy link
Member

@JeffGarland JeffGarland Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@wusatosi wusatosi Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JeffGarland JeffGarland Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JeffGarland JeffGarland Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's never too late to change things

endif()

if(BEMAN_SCOPE_BUILD_EXAMPLES)
add_subdirectory(examples)
Expand Down
8 changes: 7 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

# 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)
Copy link
Member

@JeffGarland JeffGarland Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@JeffGarland JeffGarland Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright

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.

For discussions of this library see:

Expand Down
1 change: 1 addition & 0 deletions examples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ message("Examples to be built: ${ALL_EXAMPLES}")
foreach(example ${ALL_EXAMPLES})
add_executable(beman.scope.examples.${example})
target_sources(beman.scope.examples.${example} PRIVATE ${example}.cpp)
target_link_libraries(beman.scope.examples.${example} beman::scope)
endforeach()
11 changes: 7 additions & 4 deletions examples/scope_example.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <beman/scope/scope.hpp>

#include <cstdlib>
#include <experimental/scope>
#include <iostream>
#include <string_view>

namespace scope = beman::scope;

void print_exit_status(std::string_view name, bool exit_status, bool did_throw) {
std::cout << name << ":\n";
std::cout << " Throwed exception " << (did_throw ? "yes" : "no") << "\n";
Expand Down Expand Up @@ -32,7 +35,7 @@ int main() {
// Using scope_exit: runs on scope exit (success or exception)
exit_status = did_throw = false;
try {
auto guard = std::experimental::scope_exit{[&] { exit_status = true; }};
auto guard = scope::scope_exit{[&] { exit_status = true; }};
maybe_throw();
} catch (...) {
did_throw = true;
Expand All @@ -42,7 +45,7 @@ int main() {
// Using scope_fail: runs only if an exception occurs
exit_status = did_throw = false;
try {
auto guard = std::experimental::scope_fail{[&] { exit_status = true; }};
auto guard = scope::scope_fail{[&] { exit_status = true; }};
maybe_throw();
} catch (...) {
did_throw = true;
Expand All @@ -52,7 +55,7 @@ int main() {
// Using scope_success: runs only if no exception occurs
exit_status = did_throw = false;
try {
auto guard = std::experimental::scope_success{[&] { exit_status = true; }};
auto guard = scope::scope_success{[&] { exit_status = true; }};
maybe_throw();
} catch (...) {
did_throw = true;
Expand Down
31 changes: 30 additions & 1 deletion include/beman/scope/scope.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,35 @@
#ifndef BEMAN_SCOPE_HPP
#define BEMAN_SCOPE_HPP

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

Copy link
Member

@JeffGarland JeffGarland Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wusatosi wusatosi Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wusatosi wusatosi Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JeffGarland JeffGarland Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@robert-andrzejuk robert-andrzejuk Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@JeffGarland JeffGarland Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@wusatosi wusatosi Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright!

// TODO: Implement
struct scope_exit {
template <typename F>
scope_exit(F) {}
~scope_exit() {
// TODO: Cleanup
}
};

// TODO: Implement
struct scope_fail {
template <typename F>
scope_fail(F) {}
~scope_fail() {
// TODO: Cleanup
}
};

// TODO: Implement
struct scope_success {
template <typename F>
scope_success(F) {}
~scope_success() {
// TODO: Cleanup
}
};

} // namespace beman::scope

#endif // BEMAN_SCOPE_HPP
4 changes: 2 additions & 2 deletions src/beman/scope/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
add_library(beman.scope)
add_library(beman::scope ALIAS beman.scope)

target_sources(beman.scope PRIVATE identity.cpp)
target_sources(beman.scope PRIVATE scope.cpp)

target_sources(
beman.scope
PUBLIC
FILE_SET HEADERS
BASE_DIRS ${PROJECT_SOURCE_DIR}/include
FILES ${PROJECT_SOURCE_DIR}/include/beman/scope/identity.hpp
FILES ${PROJECT_SOURCE_DIR}/include/beman/scope/scope.hpp
)

set_target_properties(beman.scope PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON)
Expand Down
2 changes: 1 addition & 1 deletion src/beman/scope/identity.cpp → src/beman/scope/scope.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <beman/scope/identity.hpp>
#include <beman/scope/scope.hpp>
8 changes: 6 additions & 2 deletions tests/beman/scope/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#add_executable(beman.scope.tests.identity)
#target_sources(beman.scope.tests.identity PRIVATE identity.test.cpp)
add_executable(beman.scope.tests.scope)
target_sources(beman.scope.tests.scope PRIVATE scope.test.cpp)

target_link_libraries(beman.scope.tests.scope PRIVATE beman::scope)

add_test(NAME beman.scope.tests.scope COMMAND beman.scope.tests.scope)
55 changes: 0 additions & 55 deletions tests/beman/scope/identity.test.cpp
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions tests/beman/scope/scope.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
int main() {
// TODO: Add tests
}