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

Latest compilers emit a ton of C4996 warnings as errors #16000

Open
Dan-Albrecht opened this issue Sep 19, 2023 · 7 comments · May be fixed by #16007
Open

Latest compilers emit a ton of C4996 warnings as errors #16000

Dan-Albrecht opened this issue Sep 19, 2023 · 7 comments · May be fixed by #16007
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products.

Comments

@Dan-Albrecht
Copy link
Member

Windows Terminal version

main @ 2e91e9c

Windows build number

10.0.22621.0

Other Software

Visual Studio Version 17.8.0 Preview 3.0 [34118.359.main]

msbuild -version
MSBuild version 17.8.0-preview-23431-02+3c910ba83 for .NET Framework
17.8.0.43102

Steps to reproduce

  • Open OpenConsole solution
  • Set to x64
  • F5

Expected Behavior

No errors

Actual Behavior

1,123 C4996 warnings as errors. e.g.:

Error	C4996	'stdext::checked_array_iterator<_Ty*>': warning STL4043: stdext::checked_array_iterator, stdext::unchecked_array_iterator, and related factory functions are non-Standard extensions and will be removed in the future. 
std::span (since C++20) and gsl::span can be used instead. 
You can define _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.	
fmt	C:\git\terminal\oss\fmt\include\fmt\format.h	359
@Dan-Albrecht Dan-Albrecht added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 19, 2023
@lhecker
Copy link
Member

lhecker commented Sep 19, 2023

Related: fmtlib/fmt#3540
We currently use 7.1.3 (2 years old? wat). In the 9 versions since then (currently 10.1.1) we've missed:

  • compile-time format string checks
  • faster format string compilation
  • "{}"_cf instead of FMT_COMPILE("{}")
  • smaller PCH
  • way faster float and hex formatting
  • way faster format_to
  • ...

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Product-Meta The product is the management of the products. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. and removed Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 19, 2023
@zadjii-msft
Copy link
Member

Yep we should probably just update that. Thanks!

@DHowett
Copy link
Member

DHowett commented Sep 19, 2023

Related: #15855

I am committing to not updating our checked-in dependencies until we have a Governance-clean pipeline for doing so :)

@lhecker
Copy link
Member

lhecker commented Sep 19, 2023

BTW I just noticed: Happy round-issue-number to you!

It turns out that the upgrade is a significant pain in the butt, because fmt largely removed support for wchar_t. Things that don't work anymore:

  • Passing wstring parameters to string formattings and vice versa
  • Implicitly casting hstring to wstring_view

I started working on this in https://github.com/microsoft/terminal/tree/dev/lhecker/16000-fmt-update

@DHowett
Copy link
Member

DHowett commented Sep 20, 2023

I am uncomfortable with the premier C++ formatting library removing support for an entire platform like that. Considering that Windows predominantly uses wide strings, that makes it nearly unfit for purpose.

Do you happen to have any links to docs about them explicitly dropping support?

@lhecker
Copy link
Member

lhecker commented Sep 20, 2023

Hey! I said "largely removed", not "dropped". 😄 And I meant it in the context of our project which uses char -> wchar_t conversions quite a bit. Maybe I should've worded it better, like "because fmt removed parts that we took a significant dependency on".

fmt now matches the behavior of std::format which doesn't provide a builtin implicit conversion either:

error C2338: static_assert failed: 'Cannot format an argument. To make type T formattable, provide a formatter<T> specialization. See N4950 [format.arg.store]/2 and [formatter.requirements].'

The same is true for winrt::hstring which also fails with the above message with std::format.

I don't know which commit(s) removed either of the two features, because there's no commit in fmt/xchar.h's history that has a fitting title, there's no issue with one either, and I'm not sure what to look for when reading the massive format-inl.h file.
But it fits the C++ standard behavior so I don't think we're missing out on anything.

@lhecker
Copy link
Member

lhecker commented Sep 20, 2023

I finished fixing all issues and as it turns out, the wchar_t / char thing was fairly minor. The biggest problem was fixing all those

format(RS_(...), ...)

which now need to be

format(fmt::runtime(RS_(...)), ...)

because it now does compile time format checks even without FMT_COMPILE (great!), but that doesn't work for hstring parameters because fmt defines its own fmt::string_view class for pre-C++17 compatibility. fmt::string_view has an implicit constructor for std::string_view, but an hstring is only implicitly convertible to an std::string_view.
Me and my C++ compiler have spent a lot of time today staring out of the window, while the "door stuck" soundtrack played in the back of my head.

@lhecker lhecker linked a pull request Sep 20, 2023 that will close this issue
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants