This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Avoid <memory> include in locale_win32.h
ClosedPublic

Authored by smeenai on Sep 8 2016, 9:38 PM.

Details

Summary

When _LIBCPP_NO_EXCEPTIONS is defined, we end up with compile errors
when targeting MSVCRT:

  • Code includes <new>
  • <new> includes <cstdlib> in order to get abort
  • <cstdlib> includes <stdlib.h>, _before_ the using ::abort
  • <stdlib.h> includes locale_win32.h
  • locale_win32.h includes <memory>
  • <memory> includes <stdexcept>
  • <stdexcept> includes <cstdlib for abort, but that inclusion gets (correctly) ignored because of header guards
  • <stdexcept> references _VSTD::abort, which isn't declared

The easiest solution is to make locale_win32.h not include <memory>,
by removing the use of unique_ptr and manually restoring the locale
instead.

Diff Detail

Event Timeline

smeenai updated this revision to Diff 70781.Sep 8 2016, 9:38 PM
smeenai retitled this revision from to [libc++] Avoid <memory> include in locale_win32.h.
smeenai updated this object.
smeenai added reviewers: compnerd, EricWF, mclow.lists.
smeenai added a subscriber: cfe-commits.

I'm aware that my replacement code isn't entirely equivalent, since it won't restore the locale in case MB_CUR_MAX throws. However, I'm quite certain the MB_CUR_MAX implementation won't throw. I can make my own RAII wrapper if desired, however.

Alternatively, MSDN documents an ___mb_cur_max_l_func, which is available Visual Studio 2013 onwards and does exactly what we want. It does warn that it's an internal CRT function and recommends against its use, hence my current implementation avoiding it. However, if we're okay using internal CRT functions, it would make for a cleaner implementation, and avoid the locale restoration issues altogether.

EricWF edited edge metadata.Sep 8 2016, 10:53 PM

LGTM. I'm OK with things that are "technically" regressions in Windows support if it helps us get it working today.

Cool. Any thoughts on this implementation vs. ___mb_cur_max_l_func? In other words, do we have a general policy on using internal CRT functionality? FWIW, support/win32/support.h was using xlocinfo.h before my recent cleanup, which is also an internal header.

bcraig added a subscriber: bcraig.Sep 9 2016, 6:05 AM

This seems related:
https://reviews.llvm.org/D20596

In that (stale) review, I switch away from unique_ptr in general for locale related operations.

smeenai planned changes to this revision.Sep 9 2016, 10:52 AM

@bcraig thanks for pointing me to that diff; there's a lot of nice cleanup going on there. Were you planning on updating and following up on it?

I also realized I forgot to adjust locale_win32.cpp for this diff. Will re-upload with that fixed.

@bcraig thanks for pointing me to that diff; there's a lot of nice cleanup going on there. Were you planning on updating and following up on it?

I also realized I forgot to adjust locale_win32.cpp for this diff. Will re-upload with that fixed.

I'm still in favor of that change, but I don't expect to have the time, resources, or permission to submit or test that change in the near future. I'm about to change jobs, and the new position doesn't involve working on the llvm projects.

smeenai updated this revision to Diff 70899.Sep 9 2016, 2:00 PM
smeenai edited edge metadata.

Correcting support_win32.cpp

@bcraig thanks for pointing me to that diff; there's a lot of nice cleanup going on there. Were you planning on updating and following up on it?

I also realized I forgot to adjust locale_win32.cpp for this diff. Will re-upload with that fixed.

I'm still in favor of that change, but I don't expect to have the time, resources, or permission to submit or test that change in the near future. I'm about to change jobs, and the new position doesn't involve working on the llvm projects.

Ah, that's unfortunate.

compnerd accepted this revision.Sep 15 2016, 11:26 AM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Sep 15 2016, 11:26 AM
This revision was automatically updated to reflect the committed changes.