msvc build fix #664
msvc build fix #664
Conversation
|
I just pushed another update to change those two template params to Also, if this is something you'd like to try to replicate on your end, the project repo I'm working with can be found here: You should just be able to open the repo dir from vscode on windows (requires the Microsoft extensions for C++ and CMake), then run the configure/build commands using the cmake extension functionality. You can also just build/configure using the cmake preset argument: |
|
Hello @vorlac .
|
|
Sorry @eyalroz - I completely missed all of your windows builds when I submitted this. Since you have it building successfully on your end without those errors I can dig a little deeper into this on my end. It's probably safe to close this since it's likely just a setup issue on my side. To answer your first question, i'm not exactly sure why msvc is complaining about this. The code in question seemed fine at a glance, but I can let you know if I uncover anything in a few days when I should free up to pick this project back up. |
Oh, no problem, on the contrary - it's important that the library get to a state where it can build on potentially-'questionable' setups; plus, if there's something which fails the build, like maybe an MSVC version - then I should be catching it at CMake configuration time, rather than allowing the build itself to fail. And it might also eventually be the case that I do have to explicitly set the type for some reason, who knows?. So, awaiting your further investigation. |
|
after some further experimenting, it seems like removing this assignment operator for poor_mans_optional& operator=(const T&& value) noexcept(::std::is_nothrow_move_assignable<T>::value) {
has_value_ = true;
maybe_value.value = ::std::move(value);
return *this;
}The ambiguity it seems to be struggling with is with the operator above, and the definition that takes in a non-cost Before messing with the The exact version of my VS installation is: other tooling used for my project's build, output directly from cmake: If you want confirmation of the VS2019 vs VS2022 build being the culprit I can install VS2019 to see if that also give me a clean build with no changes required. Or I can also change the PR to undo the previous changes and instead remove that |
|
One more update, I tried downloading and installing vs2019 to get the confirmation that the toolchain it includes differs in a way that would/could prevent this error... but I can't seem to find the installer download anywhere for the community edition. They only seem to have the professional version available for download on the site which requires a SaaS subscription. If you know of a good location to find the installer let me know and I can give it a try. It'd probably be good to get confirmation if that's truly the root cause of the build errors I'm seeing that don't show up in your builds. |
|
Can you check and see what happens if we replace |
|
The poor_mans_optional &operator=(T &&value) noexcept(::std::is_nothrow_move_assignable<T>::value)
{ return *this = value; } |
|
So, I removed it, please make sure that this addresses the issues you've seen... |
removed the const T&& constructor from poor_mans_optional
|
OK cool, I can confirm everything builds without any warnings/errors after switching the submodule I'm using over to your development branch. I also updated this PR in case you wanted an easy merge to master, but feel free to discard if your workflow is to only merge changes into master from development. |
That is indeed my workflow. When I make a release, I graft the changes from development onto master. Feel very free to make suggestions/requests even if they aren't bugs - I am always lacking concrete user feedback on this library. |
These changes just resolved some ambiguity in the assignment operator for
poor_mans_optional<>in a few locations when building on windows using msvc (should be latest version). I get the same errors from the vcpkg release as well as just including your repo as a submodule. In both cases the build is using cmake and/std:c++17.Let me know if there's a chance something might have just been misconfigured on my end to run into these build errors. Or if the changes seem necessary to resolve these errors, let me know if you'd prefer the template parameter in
pci_id.hppto beintrather thancuda::rtc::optimization_level_tand I can update the PR.Here's the output of the build prior to these changes: