syoyo / tinygltf

Header only C++11 tiny glTF 2.0 library
MIT License
2.01k stars 409 forks source link

Fix msvc build -- STRICT is a msvc macro name #450

Closed nyalldawson closed 1 year ago

nyalldawson commented 1 year ago

Follow up https://github.com/syoyo/tinygltf/pull/445

syoyo commented 1 year ago

MSVC CI build passes. https://github.com/syoyo/tinygltf/pull/445

What is the error message?

nyalldawson commented 1 year ago

Sorry, it's actually the msys build which has issues with this name. See eg https://github.com/qgis/QGIS/actions/runs/6060705066/job/16445155328

syoyo commented 1 year ago

I see. it'd be better to add msys2 + MinGW(ucrt) CI to tinygltf's Github Actions before merging this PR.

nyalldawson commented 1 year ago

Ok, https://github.com/syoyo/tinygltf/pull/451 adds that. BUT it passes, and it turns out it's not part of the mingw build itself which is defining this macro.

It must be coming from one of the dependencies in the project. (There's some hefty ones there, like Qt and GDAL which have hundreds of dependencies themselves). A quick code scan on github shows a number of projects defining this macro: https://github.com/search?q=%22%23Define+strict+%22&type=code&p=1

More details here: https://learn.microsoft.com/en-au/windows/win32/winprog/enabling-strict?redirectedfrom=MSDN

syoyo commented 1 year ago

I see.

I think it'd be better to add suffix(or prefix) to enum identifier or only capitalize the first character to avoid potential conflict of any macro names. How do you think?

  typedef enum {
    STRICTNESS_PERMISSIVE,
    STRICTNESS_STRICT,
  } ParseStrictness;

  // or

  typedef enum {
    Permissive,
    Strict,
  } ParseStrictness;
nyalldawson commented 1 year ago

@syoyo

I think it'd be better to add suffix(or prefix) to enum identifier or only capitalize the first character to avoid potential conflict of any macro names. How do you think?

Ok, done. I've gone with the capitalization change to avoid any risk.

syoyo commented 1 year ago

Brilliant! Thanks! Merged! > I've gone with the capitalization change to avoid any risk.