minio / minio-cpp

MinIO C++ Client SDK for Amazon S3 Compatible Cloud Storage
https://minio-cpp.min.io/
Apache License 2.0
136 stars 56 forks source link

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

Closed HJLebbink closed 7 months ago

HJLebbink commented 7 months ago

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.

kobalicek commented 7 months ago

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 commented 7 months ago

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.