-
Notifications
You must be signed in to change notification settings - Fork 883
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
Build errors with VS 15.3 and /permissive- conformance switch #626
Comments
@lmdsp , thank you for reporting the issue. The error message indicates the following code is invalid. include/msgpack/v1/object_fwd.hpp template <typename T>
struct has_as {
private:
template <typename U>
static auto check(U*) ->
// Check v1 specialization
typename std::is_same<
decltype(adaptor::as<U>()(std::declval<msgpack::object>())),
T
>::type;
template <typename>
static std::false_type check(...);
public:
using type = decltype(check<T>(MSGPACK_NULLPTR));
static constexpr bool value = type::value; // line 66 'value': is not a member of 'msgpack::v1::type'
}; https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/v1/object_fwd.hpp#L65 However, the type using type = decltype(check<T>(MSGPACK_NULLPTR)); Compiler doesn't use this one but tying to looking for
I think that the type |
@redboltz , thanks for your insight. According to your remarks, fixing this would be a simple matter of replacing using type = decltype(check<T>(MSGPACK_NULLPTR));
static constexpr bool value = type::value; with using t_type = decltype(check<T>(MSGPACK_NULLPTR));
static constexpr bool value = t_type::value; I'll test this tomorrow, what do you think ? |
@lmdsp , I installed Visual Studio 15.3 and got the same error. I wrote a (maybe) minimal code that reproduces the problem. See #include <type_traits>
namespace type {}
template <typename T>
struct test {
static std::true_type check();
using type = decltype(check());
static constexpr bool value = type::value;
};
int main() {
test<int>();
} I reported the issue to Microsoft (developers community). I think that it is msvc's bug.
I think that it works. In my minimal example, if I replaced I'm waiting Microsoft (developers community)'s response. If it is a bug of MSVC, I don't want to modify the code. |
The report title is |
Thanks, I'm a bit ouf of my depth on this one - I suppose I could dig in through the standard papers to check the rules of precedence / priority of namespaces over typedefs to check. I know MS have a history of sometimes 'interpreting' the standard in quite surprising ways, but lately the VS compiler team has done a great deal of work to improve conformance and stay up to date with the latest C++ 17. Anyway I've upvoted your report so it hopefully draws some attention. There seems to be a few similar reports, so hopefully this will get fixed soon. |
Hi @redboltz I seem to be having a similar issue. I am trying to use msgpack-cxx through msgpack.hpp header (
I tried both It basically complains about variadic or any other templates. My impression is you've added support for |
I think that you can try the exact same step as msgpack-c/.github/workflows/gha.yml Lines 191 to 239 in df1d126
on msvc2022 and msvc2019. Then fill the result as follows:
If msvc2019 reports the C2988 error, then the next step is checking the difference between your environment and CI(success). |
@redboltz after some investigation I found this issue is also another example of naming conflict as mentioned earlier above, but this time with other libraries macro definitions, since the function name |
I know |
It is not msvc problem, as I mentioned it is a problem for other libraries to be used with it if they have macro definitions since macros can cause ambiguity/conflicts as have precedence over functions (regardless of being private and namespaced) when processed/compiled. As I mentioned this issue was raised before in your repo, and causes major problems when used with Unreal Engine as they have check macro definition in their core engine source (i.e. |
Do you mean if someone include AssertionMacros.h, then all For example, #include <AssertionMacros.h>
int main () {
bool check = true;
if (check) {
// ...
}
} |
Yes exactly. But not a variable. Only functions. You can replicate this as below: #ifndef check
#define check(expr);
#endif
template <typename U>
static auto check(U*) ->
typename std::is_same<
decltype(adaptor::as<U>()(std::declval<msgpack::object>())),
T
>::type;
template <typename...>
static std::false_type check(...); |
I think that the macro definition of However, I consider a possible solution: your_app.cpp #include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this. The concept is all your translation units ( .cpp files) include If your code is not an application but a library, this approach should work too. E.g.) your_lib1.hpp #include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this. your_lib2.hpp #include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this. users_app.cpp #include <your_lib1.hpp>
#include <your_lib2.hpp> // msgpack.hpp is not included thanks to include guard |
This won't work since in so many libraries incl. UE, you can't change the order of includes as they're deep in the engine/library source and the way they're built and used is that you should need to have their core engine run first in order to be able to add/run your own implementation. I'm not sure why changing function name in this library is a big hassle as it's only privately used and literally no one requires it except perhaps for internal tests or am I missing something? This fix can impact a lot of Unreal game engine developers and companies since msgpack can finally be used with it without hassle of writing their own serializers to match the spec. |
Ok, if you post the PR that replaces Here are my thoughts:
It is because my policy. Easy to fix is not related. I don't accept its design. See C++ books. These kinds of macro is very bad practice. I think I've just checked similar issue is reported and found it. |
I totally understand and accept your argument. You're on the spot on this policy and I did not mean to offend. I carry the same frustration about UE and the way the market is going. Thanks for the links, yes we could use those and nuke the macros, but in this case it was much simpler to have this minor change than having to deal with a extremely bloated engine which at any point things can go very wrong (by disabling warnings etc.). Thank you for this and I'll make a PR soon. |
Thank you for the comment. I know you are not |
With Visual Studio 15.3 which has just been released, enabling the /permissive- conformance switch results in build errors.
This switch is useful to adhere strictly to the C++ standard, and it also helps enforcing cross-platform compatibility, reducing the chance to write code that will build under VS but not under clang/gcc for example.
It seems MS have enforced stricter rules as this was building fine in VS 15.2.
The error(s) appear in
example\cpp11\non_def_con_class.cpp
:The text was updated successfully, but these errors were encountered: