a few c++ fixes to allow compilation on Windows #2282
Conversation
apwojcik
added
skip bot checks
| @@ -66,7 +66,7 @@ template <class PrivateMigraphTypeNameProbe> | |||
| std::string compute_type_name() | |||
| { | |||
| std::string name; | |||
| #ifdef _MSC_VER | |||
| #if defined(_MSC_VER) && !defined(__clang__) | |||
There was a problem hiding this comment.
why does it requires check for clang. Aren't both exclusive if it is MSVC, it can't be clang.
There was a problem hiding this comment.
The Clang compiler on Windows is running in MSVC compatibility mode. It sets _MSC_VER, too. So, only checking _MSC_VER is not exclusive and is insufficient to differentiate between native MSVC and Clang. See detailed information https://clang.llvm.org/docs/MSVCCompatibility.html
Codecov Report
@@ Coverage Diff @@
## develop #2282 +/- ##
===========================================
- Coverage 91.45% 91.45% -0.01%
===========================================
Files 433 433
Lines 16175 16174 -1
===========================================
- Hits 14793 14792 -1
Misses 1382 1382
|
src/driver/main.cpp
Outdated
| @@ -39,6 +39,7 @@ | |||
| #include <migraphx/json.hpp> | |||
| #include <migraphx/version.h> | |||
|
|
|||
| #include <migraphx/instruction.hpp> | |||
There was a problem hiding this comment.
Where are we using the instruction class in this file? I cant seem to find it.
test/include/test.hpp
Outdated
| @@ -333,7 +333,7 @@ enum class color | |||
| bg_blue = 44, | |||
| bg_default = 49 | |||
| }; | |||
| inline std::ostream& operator<<(std::ostream& os, const color& c) | |||
| inline std::ostream& operator<<(std::ostream& os, [[maybe_unused]] const color& c) | |||
There was a problem hiding this comment.
I would prefer a (void)c added under #ifdef _WIN32, so we can still get diagnostics for unused variable on the linux side.
test/promote_literals_test.cpp
Outdated
| @@ -26,6 +26,7 @@ | |||
| #include <migraphx/program.hpp> | |||
| #include <migraphx/dead_code_elimination.hpp> | |||
| #include <migraphx/eliminate_common_subexpression.hpp> | |||
| #include <migraphx/instruction.hpp> | |||
There was a problem hiding this comment.
We dont use the instruction class here either, at least as far as I can tell. If windows has problems with incomplete types then maybe we should add this header in instruction_ref.hpp under #ifdef _WIN32
test/verify/run_verify.cpp
Outdated
| @@ -44,8 +44,8 @@ MIGRAPHX_DECLARE_ENV_VAR(MIGRAPHX_DUMP_TEST) | |||
|
|
|||
| // An improved async, that doesn't block | |||
| template <class Function> | |||
| std::future<typename std::result_of<Function()>::type> detach_async(Function&& f, | |||
| bool parallel = true) | |||
| std::future<typename std::invoke_result_t<Function>> detach_async(Function&& f, | |||
There was a problem hiding this comment.
Is invoke_result_t available on sles?
There was a problem hiding this comment.
Remove typename since it isn't needed anymore
apwojcik
force-pushed
the
windows_cxx_compilation
branch
from
7f30cef to
e599b93
Compare
Missing header files (mostly
<array>), a few preprocessor checks, and a portable way for checking attributes.