Rework entire code for correctness and potential bug fixes#111
Rework entire code for correctness and potential bug fixes#111balamurugana merged 83 commits intomainfrom
Conversation
HJLebbink
left a comment
There was a problem hiding this comment.
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.
balamurugana
left a comment
There was a problem hiding this comment.
Initial review.
Please fix CI failures.
|
@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 Thinking of jumping into this after I finish the stuff I'm working on. |
Except for the 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. |
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. |
Sure; I didn't know we have a linter in the CI loop. It's good news and the fixes will be promptly delivered. |
f3d2e7f to
b6ffbfd
Compare
|
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 🙂 |
|
I can extend GH actions workflow to use a much larger matrix of various configurations (OS / compiler) |
Build support for MSVC is now confirmed. The Windows warnings you can see will be fixed as a part of this PR. |
…es are the root of all evil.
… resolved the streaming bugs caused by the implicit bool conversion
…s, explicit virtual ovverrides
aa0f183 to
68fbea4
Compare
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.
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.
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.
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.
Reworked
minio-cppto in order to:NULL=>nullptrtransition, as the required language level is C++17 already.-Wall -Wextra -Wconversionand 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.