mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 54335: Add os::var() to Stout.
Date Thu, 08 Dec 2016 00:29:02 GMT


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 782
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line782>
> >
> >     I think it's worth considering whether this should be canonized and encoded
as its own function in `stout/strings.hpp` or something. It seems like we don't want to be
hand-rolling a unicode conversion in every place we need to convert from `wchar` -> `char`,
as it's super error prone. Also, it will make it easier to manage locale changes later.
> 
> Andrew Schwartzmeyer wrote:
>     Absolutely it should be. The other problem with this is that it's assuming none of
the characters in the original `wstring` were actually multi-byte. While this is *probably*
true, it is **not** guaranteed per [MSDN](https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx),
a path may:
>     > Use any character in the current code page for a name, including Unicode characters
and characters in the extended character set (128–255)
>     
>     So I'll need to fix this.

This actually turned out not to be too bad. In the new version, it's:
```
// Convert UTF-16 `wstring` to UTF-8 `string`.
std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> converter;
return converter.to_bytes(wvar_folder.c_str());
```
It's the canonical C++ way to convert UTF-16 to UTF-8 (and back if need be). If we start to
use it more we could wrap it to make it a bit cleaner looking.


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 769
> > <https://reviews.apache.org/r/54335/diff/1/?file=1575119#file1575119line769>
> >
> >     Although cumbersome, I would personally like to see this RAII'd somehow, perhaps
with a `unique_ptr` or even something incidentally RAII'd, like a `vector`.
> >     
> >     My reason for suggesting this is not just safety, but also a matter of reasoning
about the code: I tend to find it easier to reason about code when it's clear when something
is done initializing, and when it's clear what the spec for destruction is, and the easiest
way to accomplish this is to use RAII.
> 
> Andrew Schwartzmeyer wrote:
>     This is going to be interesting as the API handles the initialization... I'll see
what I can do.

Unfortunately, while `CComHeapPtr` from the ATL would be the way to go, including the ATL
headers proved to be an exercise in frustration due to our definition of `NOGDI` and a conflicing
definition of `_WINGDI_`. The alternative approach of removing `NOGDI` and fixing the root
problem it was addressing (the double definition of `ERROR`) was also fruitless, as `ERROR`
isn't the only double definition (next up was `DEBUG`). While this should be done *at some
point*, I don't think it should be done here. So I'm left with having to free the allocation
done by the C API.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158032
-----------------------------------------------------------


On Dec. 8, 2016, 12:21 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
> 
> (Updated Dec. 8, 2016, 12:21 a.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677 and MESOS-6722
>     https://issues.apache.org/jira/browse/MESOS-6677
>     https://issues.apache.org/jira/browse/MESOS-6722
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038

>   3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf

> 
> Diff: https://reviews.apache.org/r/54335/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message