Fix Aspect/Composite-relate tests on Windows/MSVC by marking every class with virtual base with appropriate pragmas #1541
Conversation
This fixes a crash on Windows. Confirmed to work in visual studio 2019. See also dartsim#1431 (comment)
traversaro
changed the title
traversaro
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #1541 +/- ##
=======================================
Coverage 58.27% 58.27%
=======================================
Files 414 414
Lines 30077 30077
=======================================
Hits 17527 17527
Misses 12550 12550
|
traversaro
changed the title
traversaro
changed the title
|
@jslee02 the history is a bit messy, after review let me know if you prefer me to squash it (I would squash it as a single commit, but let me know if you prefer a different division) or you want to squash&merge. The Travis failure seems to be related to being out of credits, and unrelated from this PR. |
There was a problem hiding this comment.
This is awesome! Don't worry about the commit history. Let me do Sqaush&merge.
Thanks for the fix @traversaro and @erwincoumans!
| if(NOT MSVC) | ||
| dart_add_test("unit" test_VskParser) | ||
| target_link_libraries(test_VskParser dart-utils) | ||
| endif() |
There was a problem hiding this comment.
Don't need to be fixed in this PR, but did you get any hint why test_VskParser doesn't build or pass with MSVC?
There was a problem hiding this comment.
I briefly saw a 0xC0000135 that typically means dll not found, so I guess it could be something related to dll of the dependencies not being in the path or not being copied in the directory with the test executables. In any case, I tried to reduce the modification as possible to have the smallest possible PR to ensure that it could be merged without side-effects.
Co-authored-by: Jeongseok Lee <jslee02@users.noreply.github.com>
…ass with virtual base with appropriate pragmas (dartsim#1541) Co-authored-by: Jeongseok Lee <jslee02@users.noreply.github.com> Co-authored-by: erwincoumans <erwin.coumans@gmail.com>
…ass with virtual base with appropriate pragmas (dartsim#1541) Co-authored-by: Jeongseok Lee <jslee02@users.noreply.github.com> Co-authored-by: erwincoumans <erwin.coumans@gmail.com>
Fix #1522 .
This fix was inspired by #1540, however just enabling the
/vd2option can change the ABI of classes with virtual base whose implementation has been compiled without the/vd2option. For this reason, using instead thevtordisppragmas explicitly for each class of Dart with a virtual base could be a more robust solution.For some reason, the fix is not working for the
Clang-Cltoolset, at least for the version of Visual Studio used in GitHub Actions. I did not investigated this, but in theory clang-cl has support for thevtordisppragmas (see https://clang.llvm.org/docs/UsersManual.html), but it is possible that this support has some bugs. However, for the time being I just remove theClang-Cltoolset from the CI jobs, and I instead enabled the unit tests that previously were failing.