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

Include headers explicitly and add constructors to comply C++20 #116

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

HJLebbink
Copy link
Member

@HJLebbink HJLebbink commented Mar 17, 2024

The constructor update is necessary for the project to compile in C++20 due to struct initialization rules made more strict in C++20. The header update is to reduce the implementations details (ic header includes) leak.

@HJLebbink HJLebbink force-pushed the feature/cpp20-support branch 8 times, most recently from 61a2494 to 37d1658 Compare March 18, 2024 10:13
@HJLebbink HJLebbink force-pushed the feature/cpp20-support branch 2 times, most recently from f2e7fa4 to 13609c1 Compare March 18, 2024 11:07
examples/PutObject.cc Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
@balamurugana balamurugana changed the title Added constructors to become C++20, fixed: implementation details leak in headers Include headers explicitly and add constructors to comply C++20 Mar 18, 2024
@kobalicek
Copy link
Contributor

kobalicek commented Mar 18, 2024

After thinking about this I would probably change the pattern used in constructors to the following:

class X {
  some_type _a;
  some_type _b;

  X(some_type a, some_type b)
    _a(std::move(a)),
    _b(std::move(b)) {}
};

The reason is that with this design the user can actually decide which argument to move and which to copy. The copy is guaranteed to only happen once, so there is only the extra move per value, which seems okay to me.

One reason to do this is move safety, for example if you have this code:

class X {
  some_type _a;
  some_type _b;

  X(some_type&& a, some_type&& b)
    _a(a),
    _b(b) {}
};

and you construct X with the same instance of some_type interesting things can happen:

some_type xxx;
X x(std::move(xxx), std::move(xxx));

What would be the content of x? Is it UB? etc...

So I would prefer the safer variant in this case.

@piotr-topnotch
Copy link
Contributor

After thinking about this I would probably change the pattern used in constructors to the following:

I concur with this recommendation as being the least intrusive.

include/credentials.h Outdated Show resolved Hide resolved
include/credentials.h Outdated Show resolved Hide resolved
include/error.h Show resolved Hide resolved
include/error.h Show resolved Hide resolved
include/error.h Show resolved Hide resolved
include/utils.h Outdated Show resolved Hide resolved
include/utils.h Outdated Show resolved Hide resolved
src/client.cc Outdated Show resolved Hide resolved
src/providers.cc Show resolved Hide resolved
src/client.cc Show resolved Hide resolved
@HJLebbink HJLebbink dismissed piotr-topnotch’s stale review March 19, 2024 14:16

Was not intended to dismiss

@HJLebbink HJLebbink merged commit e711a21 into main Mar 19, 2024
3 checks passed
@HJLebbink HJLebbink deleted the feature/cpp20-support branch March 19, 2024 14:17
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

Successfully merging this pull request may close these issues.

4 participants