Skip to content
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.

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

Consider requiring C++11 #2179

Closed
gaul opened this issue May 31, 2023 · 4 comments
Closed

Consider requiring C++11 #2179

gaul opened this issue May 31, 2023 · 4 comments

Comments

@gaul
Copy link
Member

gaul commented May 31, 2023

Currently s3fs only relies on C++03 which gives it compatibility with ancient Linux distros. Upgrading to C++11 would allow optional, span, unique_ptr, unordered_map, and some modest syntactic sugar. A bigger benefit is moving closer to compatibility with the AWS SDK (C++14) in #1068. Recently brought up in #2177.

@ggtakec
Copy link
Member

ggtakec commented Jun 10, 2023

If we don't have (or very few) environments that still have to rely on C++03, then I think it's fine to proceed with the migration to C++11.
(Because I have not been able to grasp the affected users and environments that depend on C++03, I would like to know that.)

Regarding the AWS SDK(CPP), we should consider the license of s3fs, how to link it, etc.
The only thing you want to incorporate into s3fs is around authentication, right?
(It has the same function as the library created at https://github.com/ggtakec/s3fs-fuse-awscred-lib, so it is easy to code and embed)

@ggtakec
Copy link
Member

ggtakec commented Jun 20, 2023

@gaul
During the regular build last weekend, SUSE's cppcheck has an error because cppcheck has been upgraded to 2.10.

Most of the errors say we should use the std's algorithm.
For checking the condition that it must be C++03 currenty, I tried to test using C++11(lambda function) as a trial.
As a result, only CentOS7 failed and other OSs(specified by GithubActions of the current s3fs) succeeded.

If we drop C++03 support(it means stop CentOS7 support), I can PR the current code, but if we still want to continue C++03 support, I create a PR after changing the code.

Given this situation, I think we can move to C++11.

@gaul
Copy link
Member Author

gaul commented Jun 23, 2023

RHEL7 includes GCC 4.8.x which has strong C++11 support so I think we can use most features: https://gcc.gnu.org/projects/cxx-status.html#cxx11. I think the error you encountered might just be some configuration needed, e.g., specify -std=c++11. Note that there are a bunch of other library changes that newer libstdc++ might accidentally expose in C++11 mode.

I prefer to use C++11 or newer since features like unique_ptr make it easier to write correct code but more importantly writing modern C++ makes it easier for contributors. If there is some backwards compatibility issue I think we can drop support for that distro or they can use a non-standard compiler on that distro.

The cppcheck error is confusing and maybe some false positive. I tried to look into this on my Fedora 38 desktop but compiling cppcheck has become more complicated and even Red Hat is having trouble packaging it: https://bugzilla.redhat.com/show_bug.cgi?id=2165211. Maybe it is just easier to disable newer cppcheck on a few CI runners for now?

gaul added a commit to gaul/s3fs-fuse that referenced this issue Jun 24, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References s3fs-fuse#2179.
gaul added a commit to gaul/s3fs-fuse that referenced this issue Jun 24, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References s3fs-fuse#2179.
gaul added a commit to gaul/s3fs-fuse that referenced this issue Jul 24, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References s3fs-fuse#2179.
gaul added a commit to gaul/s3fs-fuse that referenced this issue Jul 26, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References s3fs-fuse#2179.
gaul added a commit to gaul/s3fs-fuse that referenced this issue Jul 26, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References s3fs-fuse#2179.
ggtakec pushed a commit that referenced this issue Jul 27, 2023
This simplifies code paths and makes memory leaks less likely.  It
also makes memory ownership more explicit by requiring std::move.
This commit requires C++11.  References #2179.
@gaul gaul closed this as completed Jul 31, 2023
@gaul
Copy link
Member Author

gaul commented Jul 31, 2023

1.93 will require C++11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants