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

Guard against usage of _isatty when header was not included #3880

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

mtillmann0
Copy link
Contributor

The recent addition of the FMT_WINDOWS_NO_WCHAR flag tried to enable builds for Windows platforms that are not WINAPI_FAMILY_DESKTOP_APP as per the WINAPI family.
The inclusion of <io.h> became conditional on FMT_WINDOWS_NO_WCHAR. The usage of _isatty from that header however was not guarded. This PR tries to fix this.

There are two followup points for discussion:

  • The name of FMT_WINDOWS_NO_WCHAR is incredibly misleading since it has nothing to do with wchar support but with the availability of the console output function WriteConsoleW only on WINAPI_FAMILY_DESKTOP_APP platforms. Should that macro be renamed?
  • The need for FMT_WINDOWS_NO_WCHAR could be auto-detected by querying the WINAPI_FAMILY macros provided by the Windows header. Should this be included in format.h with the other config detection?

@vitaut
Copy link
Contributor

vitaut commented Mar 4, 2024

As far as I remember from #3636 autodetection turned out to be problematic/messy. Not opposed to renaming, do you have good suggestions? cc @glebm

@mtillmann0
Copy link
Contributor Author

mtillmann0 commented Mar 4, 2024

I would prefer an auto-detection in the header instead of the build tools something along the lines of

#ifdef WINAPI_FAMILY_PARTITION
  #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_SYSTEM)
    #define FMT_WINDOWS_NO_WCHAR
  #endif
#endif

This would necessitate the inclusion of the windows SDK header, which might be unwanted? I see that the windows header for WriteConsoleW was also explicitly not included instead opting to declare the function inside of fmt.

For the naming I would propose FMT_WINDOWS_DESKTOP_OR_SYSTEM if it is coupled directly to the meaning above.

edit: here is the link with the winapi guards for WriteConsoleW: https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/um/consoleapi.h#L45

@glebm
Copy link
Contributor

glebm commented Mar 5, 2024

The recent addition of the FMT_WINDOWS_NO_WCHAR flag tried to enable builds for Windows platforms that are not WINAPI_FAMILY_DESKTOP_APP as per the WINAPI family.

No, the flag was actually for Windows 98 support. I had no idea that it'd also be useful for some modern Windows variants.

I don't think WINAPI_FAMILY_PARTITION is available on Windows 98. Autodetection was indeed rather messy (see discussion in #3636). If autodetection would be limited to modern platforms, I guess that'd be fine.

For the naming I would propose FMT_WINDOWS_DESKTOP_OR_SYSTEM if it is coupled directly to the meaning above.

That's not what this flag actually is. If you really want to rename it, perhaps FMT_WINDOWS_USE_WRITE_CONSOLE_W or similar?

@mtillmann0
Copy link
Contributor Author

Thanks for clarifying the original intent. So there are overlapping but distinct motivations.

@vitaut
Copy link
Contributor

vitaut commented Mar 5, 2024

Adding windows includes to format.h is undesirable. I suggest renaming the macro to FMT_USE_WRITE_CONSOLE.

@mtillmann0
Copy link
Contributor Author

For me, and I assume others using these modern platforms, this fine enough. I will rename the macro in this PR if there is no further disagreement on FMT_USE_WRITE_CONSOLE and drop the idea of detecting the platform for now.

@vitaut vitaut merged commit 13cfaa2 into fmtlib:master Mar 8, 2024
41 checks passed
@vitaut
Copy link
Contributor

vitaut commented Mar 8, 2024

Thank you!

happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
)

* Guard against usage of _isatty when header was not included

* Rename FMT_WINDOWS_NO_WCHAR macro to FMT_USE_WRITE_CONSOLE
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 this pull request may close these issues.

3 participants