Skip to content

int: Remove deprecated std::codecvt from strutil.cpp internals#5107

Open
lgritz wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
lgritz:lg-codecvt
Open

int: Remove deprecated std::codecvt from strutil.cpp internals#5107
lgritz wants to merge 1 commit intoAcademySoftwareFoundation:mainfrom
lgritz:lg-codecvt

Conversation

@lgritz
Copy link
Copy Markdown
Collaborator

@lgritz lgritz commented Mar 22, 2026

For the sake of Windows' need for wstring filenames, we do some utf8<->utf16 conversion in strutil.cpp. That has always used the std::codecvt set of functions, but they have been deprecated beginning in C++17 and will be removed entirely in C++26, so their days are numbered and we already have to suppress warnings to get the compiler to shut up about it.

Assisted-by: Claude Code / Opus 4.6

For the sake of Windows' need for wstring filenames, we do some
utf8<->utf16 conversion in strutil.cpp. That has always used the
std::codecvt set of functions, but they have been deprecated beginning
in C++17 and will be removed entirely in C++26, so their days are
numbered and we already have to suppress warnings to get the compiler
to shut up about it.

Assisted-by: Claude Code / Opus 4.6

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@kmilos
Copy link
Copy Markdown

kmilos commented Apr 1, 2026

Now that the long path manifest was added in #5066, one could also force UTF-8 process code page from it, so all the wstring stuff could just be removed in some future OIIO version once support for Windows 8.1 and earlier is deemed not to be required any longer.

See e.g. https://github.com/AOMediaCodec/libavif/blob/70eb3fcb49d4e9ee6dde26553cfc3bc673b081e7/apps/utf8.manifest

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Apr 1, 2026

Now that the long path manifest was added in #5066, one could also force UTF-8 process code page from it, so all the wstring stuff could just be removed in some future OIIO version once support for Windows 8.1 and earlier is deemed not to be required any longer.

My understanding is that the manifest thing only applies to executables. We can do it for the executables we build like oiiotool, but we don't have a way to make this the case for the libOpenImageIO library that might be linked by some downstream app. No?

@kmilos
Copy link
Copy Markdown

kmilos commented Apr 1, 2026

That's correct, it would require any OIIO library clients to do the same, so it does need to be a careful, well timed, and clearly communicated decision of breaking with the legacy.

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Apr 1, 2026

But in the mean time, does anybody have objections to the changes in this PR?

@kmilos
Copy link
Copy Markdown

kmilos commented Apr 2, 2026

Is this used anywhere else except Windows? It shouldn't be, right? If so, why not simply remove all that code and only leave the _WIN32 paths w/ already native MultiByteToWideChar/WideCharToMultiByte? I don't believe a custom conversion implementation is necessary (increasing surface for bugs etc), nor that this should be part of the API - address the root cause, not the warning.

P.S. If this means waiting until the next soversion bump, so be it. IMHO still better to live w/ a warning for a few more months (it's not like they will remove codecvt tomorrow) than to introduce cruft... See also AcademySoftwareFoundation/openexr#2314

P.P.S. Shoot, looks like there is one single place really needing this on all platforms:

spec.attribute(tagname, Strutil::utf16_to_utf8(wstr));
Everything else wstring related really ought to be guarded by _WIN32 and taken out of the API as much as possible IMHO...

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Apr 2, 2026

Everything else wstring related really ought to be guarded by _WIN32 and taken out of the API as much as possible IMHO...

Yes, but that's not a change we can make until the next major release (4.0, years away). We don't remove public API calls for minor (yearly) or patch (monthly) releases. So for now, we do need this fix.

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.

2 participants