CMake: fix MSVC __cplusplus

[FranciscoPombal]

---

See this comment onward in the libtorrent issue tracker: arvidn/libtorrent#5114 (comment).

Basically, this prevents deprecation warnings due to erroneous values of the __cplusplus macro on MSVC. See https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ for more info. The only reason this is not default is because otherwise it would break legacy code relying on MSVC to always setting this to the same value.

I realize the commit title might not be the best, open to suggestions.

[Chocobo1]

First, IIRC qbt code doesn't use this macro, so we don't actually need it nor do anything about it.
Second, IIRC we always use the latest default c++ version on MSVC and therefore no need to rely on this macro.

[glassez]

IIRC qbt code doesn't use this macro

Even if qbt doesn't use this macro directly it still might be used indirectly, e.g. if libtorrent uses it in headers. But in this case, we should not touch it manually, and use exactly the same value as libtorrent, otherwise we may get conflicts.

[Chocobo1]

But in this case, we should not touch it manually,

I would expect libtorrent to propagate it in cxx flags in the case of cmake.

[FranciscoPombal]

@glassez @Chocobo1

It's still useful for our own code. /Zc:__cplusplus guarantees that the correct macro value is used for each standard. It should have always been the correct value that corresponds to the standard, unless we were already relying on it being incorrect, which fortunately is not the case. This flag fixes an old Microsoft mistake, and the only reason it's not on by default is because of legacy code.

Right now, we don't seem to have any problem due to the macro taking on the wrong value, but I think that's because we are just lucky. Consider that MSVC uses C++14 by default: https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-160 - if we are to switch to C++17 soon in our code base, there will again be a mismatch between the macro and the std switch. Best case scenario, it only leads to a few harmless deprecation warnings during the build. Worst case, who knows. Using /Zc:__cplusplus guarantees we'll get the correct behavior always, so we don't have to coast on luck.

As far as compatibility with libtorrent goes: there is no problem because of exactly this:

I would expect libtorrent to propagate it in cxx flags in the case of cmake.

But if something is important for our own code, we should not rely on a dependency to propagate it to exempt us from declaring it ourselves. Propagation of requirements is to be relied upon for requirements that are specific to the dependency only, not general ones that happen to coincide.

[glassez]

@FranciscoPombal, why are you always so needlessly verbose? I just meant that we can safely enable this flag only if it is enabled when compiling all the linked libraries.

[FranciscoPombal]

@glassez

@FranciscoPombal, why are you always so needlessly verbose? I just meant that we can safely enable this flag only if it is enabled when compiling all the linked libraries.

And I'm saying that we can and should safely enable it because there is no dependency that we consume, nor any piece of our own code, that assumes the macro is hardcoded to the wrong value. The dependencies we use actually employ it for feature detection (which is its intended purpose anyway)[1], so we ought to make sure it is set to the correct value as well. Only very old legacy code assumes this macro is hardcoded to the wrong value and will break when this option is passed. Maybe you should investigate a little yourself instead of complaining about other people taking their time to explain things clearly to you.

[1]: an example in libtorrent: https://github.com/arvidn/libtorrent/blob/b029d4f296df5ca53c5fce24cd781c235e2484ac/include/libtorrent/units.hpp#L160. This is why arvidn/libtorrent#5336 makes sure the correct code will be used when building with CMake and MSVC.

[glassez]

Maybe you should investigate a little yourself instead of complaining about other people taking their time to explain things clearly to you.

Maybe you should discuss exactly the aspects of the problem that you are asked about, instead of thinking everyone around you is a moron who needs to be explained everything from scratch.

an example in libtorrent: https://github.com/arvidn/libtorrent/blob/b029d4f296df5ca53c5fce24cd781c235e2484ac/include/libtorrent/units.hpp#L160. This is why arvidn/libtorrent#5336 makes sure the correct code will be used when building with CMake and MSVC.

Doesn't your example confirm my concerns?
Let's say we merged this PR. Now try to imagine what will happen with the code from your example when we compile qBittorrent with libtorrent-1.2.11 (current version), which has no changes from arvidn/libtorrent#5336.

[FranciscoPombal]

@glassez

Maybe you should discuss exactly the aspects of the problem that you are asked about, instead of thinking everyone around you is a moron who needs to be explained everything from scratch.

This is frankly a stupid assumption. I don't explain things because I think you're a moron. There is nothing to lose by explaining things clearly. If you understand what is said, and even think the explanation is too verbose for you, good for you, no need to feel insulted (why would you?). In fact, a clear explanation benefits lurkers and other participants who may come come across this and not be as knowledgeable as you. This increases the chance that a third party might contribute something useful for the discussion that they otherwise wouldn't have been able to because of trouble understanding a new subject. These kinds of reactions against clear explanations make those less knowledgeable feel inadequate - they will avoid participating because they will fear being judged too harshly by you through no fault of their own. This creates a negative environment for information sharing.

Doesn't your example confirm my concerns?
Let's say we merged this PR. Now try to imagine what will happen with the code from your example when we compile qBittorrent with libtorrent-1.2.11 (current version), which has no changes from arvidn/libtorrent#5336.

I tested briefly on Windows, and IIRC the result was a few compilation warnings (or maybe I tested the other combination, libtorrent without arvidn/libtorrent#5336 and qBittorrent with this patch). Basically the warnings that were appearing on libtorrent were "moved up" to qBittorrent compilation time. So I'd say it's not much of a regression, but not much of an overall improvement either. I still think the likelihood of any kind of problem arising from such a combo is extremely low, but yeah, why risk it? I agree we have more to gain by waiting for the 1.2.12 release before merging this.

[FranciscoPombal]

@glassez @Chocobo1

As per the discussions in arvidn/libtorrent#5114, this change was also implemented in libtorrent: arvidn/libtorrent#5336 (long ago now, I think the first tagged release that included this was 1.2.12). Now it's our turn, as none of the hypothetical concerns that were raised came to fruition.