bkaradzic / bgfx

Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
https://bkaradzic.github.io/bgfx/overview.html
BSD 2-Clause "Simplified" License
14.59k stars 1.92k forks source link

BX and BGFX do not support the conformant MSVC preprocessor #3265

Closed Zarklord closed 3 months ago

Zarklord commented 3 months ago

Describe the bug There are several macro that are defined slightly differently, because MSVCs preprocessor was not conforming to standards. About 2 years ago, MSVC has supported a standards compliant preprocessor by setting the build flag /Zc:preprocessor, at some point in the future, I believe the conforming preprocessor will be on by default.

https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview?view=msvc-170

To Reproduce Steps to reproduce the behavior:

  1. Add the /Zc:preprocessor to your build options
  2. Build

Expected behavior The Macro's and code defined to work around bugs should only be defined if the traditional MSVC preprocessor is enabled

#if !defined(_MSVC_TRADITIONAL) || _MSVC_TRADITIONAL
// Logic using the traditional preprocessor
#else
// Logic using cross-platform compatible preprocessor
#endif
bkaradzic commented 3 months ago

Don't know how this is related to bx or bgfx? What's the actual issue here?!

Zarklord commented 3 months ago

If you enable the confirming preprocessor, bx and bgfx cannot compile

bkaradzic commented 3 months ago

But what's the issue?

What about GCC and Clang, they don't have conforming processor?

Zarklord commented 3 months ago

The issue is you have preprocessor macros that only run on msvc (that overcome the issues), precisely because msvc has a non conforming preprocessor, like this one: https://github.com/bkaradzic/bx/blob/dc3bf2990e62e8e8764de1f430637aa26e333296/include/bx/macros.h#L14

mcourteaux commented 3 months ago

I think he's saying that the workarounds are no longer needed with an up-to-date version of MSVC (potentially after adding the /Zc:preprocessor flag. Not really an issue if you ask me, but more of a hint that these can be refactored in the future.

Zarklord commented 3 months ago

I think he's saying that the workarounds are no longer needed with an up-to-date version of MSVC (potentially after adding the /Zc:preprocessor flag. Not really an issue if you ask me, but more of a hint that these can be refactored in the future.

Nope, you actually can't ever refactor these out, unless you want to drop support for all MSVC compilers older than VS 2017 V15.8 (and that don't have the flag set), additionally, leaving it alone will prevent this from compiling on C++20 for MSVC (or if the flag has been set) because the current MSVC macros workarounds break during compilation with a conforming preprocessor.

Because of the cpp world we live in, the msvc macro workarounds just need to check if the non conforming preprocessor is also enabled, otherwise you lose backwards or forwards compatibility.

bkaradzic commented 3 months ago

leaving it alone will prevent this from compiling on C++20 for MSVC

bx/bgfx/etc. is only C++17 right now. ETA for C++20 adoption is sometime in 2025.

At some point I'll just flip the switch to new MSVC conforming preprocessor. Until then we assume it doesn't have conforming preprocessor.

Zarklord commented 3 months ago

leaving it alone will prevent this from compiling on C++20 for MSVC

bx/bgfx/etc. is only C++17 right now. ETA for C++20 adoption is sometime in 2025.

At some point I'll just flip the switch to new MSVC conforming preprocessor. Until then we assume it doesn't have conforming preprocessor.

Unless I'm misreading something in GENie, BGFX compiles as c++14 on MSVC...

But also why are you even against adding a few lines of code to support a compiler option that you're going to have to support next year anyway?

bkaradzic commented 3 months ago

Unless I'm misreading something in GENie, BGFX compiles as c++14 on MSVC...

https://github.com/bkaradzic/bx/blob/3072cf37dfb4b91da2ac057815ee8f0b209424d8/scripts/toolchain.lua#L209

But also why are you even against adding a few lines of code to support a compiler option that you're going to have to support next year anyway?

Because every separate code path adds maintenance cost... Once I decide it's time to support it, I'll spend time to remove old code paths.

Zarklord commented 3 months ago

https://github.com/bkaradzic/bx/blob/3072cf37dfb4b91da2ac057815ee8f0b209424d8/scripts/toolchain.lua#L209

Ah, I missed that because I forgot to check BX.

Because every separate code path adds maintenance cost... Once I decide it's time to support it, I'll spend time to remove old code paths.

But it's not a separate code path, it's adding an extra condition to the couple places that have this divergent behavior, the only way that this would add a new line of code is if you chose to wrap the checking in a "BX_HAS_TRADITIONAL_MSVC_PREPROCESSOR" macro or something like that, either way your just &&-ing an extra conditional(s) onto the couple places with the already if elsed MSVC traditional preprocessor behavior.

bkaradzic commented 3 months ago

@Zarklord you're wasting my time... I don't have time to bikeshed about some nonsense. If you need this, you can fork bx/bgfx and modify it for yourself.

I don't see the issue with current setup.

Zarklord commented 3 months ago

@Zarklord you're wasting my time... I don't have time to bikeshed about some nonsense. If you need this, you can fork bx/bgfx and modify it for yourself.

I don't see the issue with current setup.

It's ironic that your complaining about not having time to talk about this when it would have taken less time to fix the problem than write that comment... Regardless I look forward to you making this change anyway in like 6 months to a year when you go to support c++20 👍🏻.