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

Rework entire code for correctness and potential bug fixes #111

Merged
merged 83 commits into from
Mar 16, 2024

Conversation

piotr-topnotch
Copy link
Contributor

@piotr-topnotch piotr-topnotch commented Mar 5, 2024

Reworked minio-cpp to in order to:

  1. Fix all the bugs caused by implicit conversions to bool.
  2. Defaulted constructors and destructors.
  3. Explicit override indicators of virtual functions.
  4. Enhanced copy elision and enabled move construction.
  5. Improved const correctness. Only the functions that mutate objects take mutable references.
  6. NULL => nullptr transition, as the required language level is C++17 already.
  7. Fixed a number of the lack of default initialization value bugs.
  8. Enforced -Wall -Wextra -Wconversion and fixed the relevant uncompilable sites.

The scope of the changes is limited to what is allowed by the requirement to leave the API unchanged. Nonetheless, a follow-up pass removing converting constructors is highly recommended.

Copy link
Member

@HJLebbink HJLebbink left a comment

Choose a reason for hiding this comment

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

LGTM, Large and absolutely needed update., much appreciated. I ran the tests on WSL2. But I'm in no position to predict the impact on upstream projects that use this repo. @harshavardhana @balamurugana, or anyone in such position, could you please have a look. I need these changes for a POC. @piotr-topnotch if there is no resolution for this PR in the coming weeks, could you please consider to fork to prevent a stall our other efforts.
Note, the PWTODO will need updates but please not in this PR.

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Initial review.

Please fix CI failures.

include/args.h Outdated Show resolved Hide resolved
include/args.h Outdated Show resolved Hide resolved
include/client.h Show resolved Hide resolved
include/client.h Show resolved Hide resolved
include/utils.h Outdated Show resolved Hide resolved
src/args.cc Outdated Show resolved Hide resolved
src/args.cc Outdated Show resolved Hide resolved
src/args.cc Outdated Show resolved Hide resolved
src/credentials.cc Show resolved Hide resolved
src/utils.cc Show resolved Hide resolved
@kobalicek
Copy link
Contributor

@piotr-topnotch I think this needs Windows fixes as well.

I assume this is the first PR that doesn't introduce API breaks? Because all the passing of stuff via value (like std::string, etc...) that has to go away. I would suggest std::string_view for everything that doesn't need to be retained (that would be a good start) and then I guess we will have to evaluate the API one by one to make it nicer.

Thinking of jumping into this after I finish the stuff I'm working on.

@piotr-topnotch
Copy link
Contributor Author

I assume this is the first PR that doesn't introduce API breaks?

Except for the explicit operator bool() which is the root of most problems here, that's precisely the goal here. More fixes are pending, but let's do it one step at a time.

Moreover, I spoke to @kannappanr and he agreed to treat the removal of all the implicit conversions as bugfixes, even if these changes would break comparability at the user end. As the user experience has priority, this enables much deeper rework.

@piotr-topnotch
Copy link
Contributor Author

But I'm in no position to predict the impact on upstream projects that use this repo.

Thank you, @HJLebbink. @kannappanr authorized breaking changes if that improves user experience and can help fix bugs. There shouldn't be many in this PR, but there are more issues requiring our attention, e.g. implicit conversions.

@piotr-topnotch
Copy link
Contributor Author

Please fix CI failures.

Sure; I didn't know we have a linter in the CI loop. It's good news and the fixes will be promptly delivered.

@astrocox
Copy link
Contributor

astrocox commented Mar 8, 2024

Not sure if you have easy access to a Windows dev environment. If not, lmk, I might be able to take a look at the CI failures this weekend.

@HJLebbink
Copy link
Member

HJLebbink commented Mar 8, 2024

Not sure if you have easy access to a Windows dev environment. If not, lmk, I might be able to take a look at the CI failures this weekend.

@astrocox I wanted to contact you: have you been able to compile and link this project in the past on WSL2 or on native Windows? I and @piotr-topnotch have such win boxes, but use WSL most of the time. For community support, windows native support would be a very good thing to have.

@astrocox
Copy link
Contributor

astrocox commented Mar 8, 2024

Not sure if you have easy access to a Windows dev environment. If not, lmk, I might be able to take a look at the CI failures this weekend.

@astrocox I wanted to contact you: have you been able to compile and link this project in the past on WSL2 or on native Windows? I and @piotr-topnotch have such win boxes, but use WSL most of the time. For community support, windows native support would be a very good thing to have.

Native windows, after my changes from #108 ! You can check the windows build pipeline for the steps to install dependencies. Locally I have tested it with cl.exe and clang-cl.exe, but the GH actions build is using the regular MSVC cl.exe compiler. If you're using an IDE like Clion, it's pretty easy to set up two different cmake profiles that use Visual Studio and WSL as their respective toolchain.

There are still a lot of compiler warnings to clean up on windows, but I think some Windows support is better than none 🙂

@kobalicek
Copy link
Contributor

I can extend GH actions workflow to use a much larger matrix of various configurations (OS / compiler)

@piotr-topnotch
Copy link
Contributor Author

There are still a lot of compiler warnings to clean up on windows, but I think some Windows support is better than none 🙂

Build support for MSVC is now confirmed. The Windows warnings you can see will be fixed as a part of this PR.

examples/BucketExists.cc Outdated Show resolved Hide resolved
src/baseclient.cc Outdated Show resolved Hide resolved
src/baseclient.cc Outdated Show resolved Hide resolved
@balamurugana balamurugana changed the title Feature/rework Rework entire code for correctness and potential bug fixes Mar 16, 2024
@balamurugana balamurugana merged commit 765ec24 into main Mar 16, 2024
3 checks passed
@harshavardhana harshavardhana deleted the feature/rework branch March 16, 2024 01:20
flokaiser pushed a commit to OroraTech/minio-cpp that referenced this pull request Jul 12, 2024
this flag assignement was removed in:
  minio#111

I don't know if it was removed intentional, but without it
the BaseUrl.https flag only reflects the state of how BaseUrl was
constructed with.
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.

5 participants