Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All-units prebuilt header error on pascal on MSVC #284

Closed
connorjak opened this issue Aug 12, 2024 · 6 comments · Fixed by #285
Closed

All-units prebuilt header error on pascal on MSVC #284

connorjak opened this issue Aug 12, 2024 · 6 comments · Fixed by #285

Comments

@connorjak
Copy link

When using https://aurora-opensource.github.io/au/0.3.5/au_all_units.hh with my C++20 project, I get these errors, only on the pascal definition. I was originally on a different commit version of the file, and after the change to 0.3.5 proper it had the exact same errors. If I rename pascal to pascal2, the error goes away. Maybe there's a shadowed type, keyword, or global here, somehow?

Au\au_all_units.h(6087): error C2059: syntax error: '='
Au\au_all_units.h(6087): error C2143: syntax error: missing ';' before '{'
Au\au_all_units.h(6087): error C2447: '{': missing function header (old-style formal list?)

Compile info:

  • Windows 11
  • RelWithDebInfo Configuration
  • Toolchain: msvc_x64_64
  • MSVC v143 - VS 2022 17.10.3
  • cl version 14.40
  • CMake build through Visual Studio's CMake support (Ninja)
  • CMake configuration:
set(CMAKE_CXX_STANDARD 20) 
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
@connorjak
Copy link
Author

Reproduced it in a simpler project with C++17. Windows macros strike again!
image

@connorjak
Copy link
Author

#undef pascal after including Windows.h or similar does the trick.

@chiphogg
Copy link
Contributor

A lowercase-named macro? The monsters!

I think we'll have to handle this the same way we did PI. That is, we'll [[deprecate]] it and stick it inside of an #ifndef pascal block for now, and then remove it outright once it's been deprecated for a full minor release version.

Thanks for finding the problem and tracking down the root cause!

chiphogg added a commit that referenced this issue Aug 12, 2024
It conflicts with the `pascal` macro in `<Windows.h>`.  Yes, they really
did define a lowercase-named macro in `<Windows.h>`.

As with `PI` (#247), the `#ifndef` should immediately unblock most
users, but it's not a long-term solution, because it depends on
order-of-includes.  Therefore, we also deprecate `pascal` (as with `PI`
in #250).  I think a good rule of thumb is that deprecated constructs
should be deprecated for at least one full minor release cycle, so we'll
plan to delete it (along with `PI`) as part of 0.5.0.

Fixes #284.
chiphogg added a commit that referenced this issue Aug 12, 2024
It conflicts with the `pascal` macro in `<Windows.h>`.  Yes, they really
did define a lowercase-named macro in `<Windows.h>`.

As with `PI` (#247), the `#ifndef` should immediately unblock most
users, but it's not a long-term solution, because it depends on
order-of-includes.  Therefore, we also deprecate `pascal` (as with `PI`
in #250).  I think a good rule of thumb is that deprecated constructs
should be deprecated for at least one full minor release cycle, so we'll
plan to delete it (along with `PI`) as part of 0.5.0.

Fixes #284.
@chiphogg
Copy link
Contributor

I thought we had fixed this in #285, but this compiler explorer link suggests it may still be a problem. Adding #undef pascal after "Windows.h" appears to still be necessary.

Did we miss something?

@chiphogg chiphogg reopened this Aug 12, 2024
@connorjak
Copy link
Author

I thought we had fixed this in #285, but this compiler explorer link suggests it may still be a problem. Adding #undef pascal after "Windows.h" appears to still be necessary.

Did we miss something?

I don't use compiler explorer very often; can you show me what is going wrong?

@chiphogg
Copy link
Contributor

Well, that is very strange. Nothing appears to be wrong anymore. Although really, I guess the strange part is that it appeared to be broken after landing in the first place --- we really did expect this PR to fix the issue.

If you want to see what the problem was, you can replace main with 0.3.5 in the #include URL for Au. The error I'm getting there looks the same as what I had been seeing before.

In any case, I think this is fixed for real now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants