Compiling MuseScore with Visual Studio 2017

• May 2, 2018 - 06:40

I've started working on issue #271135: Compile MuseScore with msvc / Visual Studio. I've already made the first PR, with initial changes allowing MuseScore to compile with no errors with the MSVC toolchain, but this is still WIP.

Current status

On the positive side:
- CMake system creates solution and projects for MSVC succesfully,
- All the projects comprising MuseScore compile successfully, and generate an executable,
- All required DLLs are copied to the output directory.

On the negative side:
- Compiling with warning level 4 (/W4) the solution still has 1257 unsolved warnings (see below),
- When run, the program stops with an exception (see below).
- The INSTALL CMake target has not been ported to the MSVC toolchain yet.

Unsolved warnings

The unsolved warnings fall in several categories:
- Name hiding; many of the warnings are due to variable name reuse, where a variable defined in an inner scope hides a variable in an outer scope (or a function parameter, or a class field). To solve these, the variables should be renamed. This is a good practice anyway, in keeping with the recommendation ES.12 in the C++ Core Guidelines by Stroustrop & Sutter (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-reuse : ES.12: Do not reuse names in nested scopes).
- Some cases of name hiding are due to duplicated variable names introduced by the macro-expansion of Qt foreach() loops. To solve these, the loops should be converted to standard C++ for(:) loops, as per the "MuseScore Coding Rules" (https://musescore.org/en/handbook/musescore-coding-rules#Loops), which tells explicitly "Use C++11's "for" instead of Qt's "foreach":" ... "If you happen to be fixing some code and see a "foreach", please change that loop into a "for"."
- Unreachable code. To solve, either eliminate the code, or conditionally exclude for MSVC builds.
- Narrowing conversions, for example from double to float. To solve, explicitly cast the result to the desired type.
- Switch statements with default, but no case labels: this is probably WIP, where the switch will be later populated. If this is the case, this warning could be selectively disabled.
- (Potentially) uninitialized variables. This indicates that MSVC could not determine if the variable was initialized prior to its first use. To solve, initialize variables.
- Assignment within conditional expression. This is not a problem per-se, but can lead to bugs. To solve, separate the assignment as a separate statement.
- There are other, less frequent warnings as well.

Exception on execution

When executing the program, an exception is triggered early in the initialization process (line 343 - libmscore/style.cpp, the QVariant::fromValue() method is throwing an exception. All QVariant::fromValue() invocations for the enum Align work flawlessly. The only difference I've noticed is that for Ms::Align, Q_DECLARE_METATYPE(Ms::Align) is invoked in elementlayout.h, whereas for Direction this is not used, and Q_ENUM_NS(Direction) is. Could this be the source of the problem? ).

Comments

Jojo-Schmitz

• May 2, 2018 - 07:24

Just some random remarks.

In the issue @lasconinc mentiones:
in all.h, #define WIN32_LEAN_AND_MEAN to avoid some name clashes with Windows headers
You're not doing that?

In your code you use
#if (!defined (MSCVER) && !defined (_MSC_VER))
while @lasconic suggested
_We can use Q_CC_MSVC to write specific code for MSVC

As to unreachable code: IMHO better remove it altogether.

As to potentially uninitialized variable: IMHO initialize unconditionally, shouldn't harm with other compilers

As to unused parameters or local variable: better use Q_UNUSED(var);or just remove the variable name from the function header rather than invent your own marco for this

afprado

• May 2, 2018 - 20:45

WIN32_LEAN_AND_MEAN excludes APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets. It doesn't help with compatibility or name clashes, but it might accelerate compilation a little bit (although with pre-compiled headers the effect should be negligible). Doesn't seem worthwhile...

Using Q_CC_MSVC is the same as using _MSC_VER. The Qt header file defines Q_CC_MSVC conditionally whenever _MSC_VER is defined (qcompilerdetection.h, lines 85 - 89).

#elif defined(_MSC_VER)
#  ifdef __clang__
#    define Q_CC_CLANG ((__clang_major__ * 100) + __clang_minor__)
#  endif
#  define Q_CC_MSVC (_MSC_VER)
........

I prefer the latter, because it is independent of the Qt library, and can be applied in modules that do not depend on Qt. Additionally, in all.h, the first conditional compilation is before the Qt headers are included, and Q_CC_MSVC is not yet defined there.

Unreachable code: agree. I would just comment it out, so that the original code is still visible.

Potentially uninitialized variables: agree, it's a good practice to initialize on definition.

Unused vars/parameters: Q_UNUSED() is the way to go when there is a Qt dependency, as this will work for all platforms; otherwise, the UNREFERENCED_PARAMETER() macro, defined in winnt.h, is an equivalent substitute (I copied the definition from winnt.h in one instance for a file which was not including either Qt nor winnt.h, to avoid generating a false dependency just for this macro, could copy the Q_UNUSED() macro definition instead). As a reference, the definitions are almost identical:

#define Q_UNUSED(x) (void)x;
#define UNREFERENCED_PARAMETER(P)          (P)