mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 54335: Add `os::var()` to Stout.
Date Thu, 08 Dec 2016 06:14:53 GMT

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




3rdparty/stout/include/stout/windows/os.hpp (line 20)
<https://reviews.apache.org/r/54335/#comment229265>

    Tiny nit: while you're messing around with the headers, could we alphabetize them?



3rdparty/stout/include/stout/windows/os.hpp (line 756)
<https://reviews.apache.org/r/54335/#comment229266>

    I think it's worth mentioning specifically something like "will not cause us to return
different directories for different users." I think when I read this in x years it will not
be super clear to me what you mean when you say "will not change the result." :) Change the
result, with respect to what?



3rdparty/stout/include/stout/windows/os.hpp (line 754)
<https://reviews.apache.org/r/54335/#comment229264>

    Mesos style is to use `wchar_t* var_folder` rather than `wchar_t *var_folder`.
    
    Also, smaller question: is there a particular reason we switched from `PWSTR` to `wchar_t`?
If not, it seems to me that if the win32 API is going to go through the trouble of using opaque
typedefs, we should use those when we call their functions, too. (And convert to something
more appropriate to Mesos before returning any data.) Here and elsewhere. Or, alternatively,
we might consider putting a `static_assert` into Windows to make sure these are the same type.
(Perhaps we should do this anyway since I believe the `wstring` constructor below doesn't
take `PWSTR`.)



3rdparty/stout/include/stout/windows/os.hpp (lines 757 - 760)
<https://reviews.apache.org/r/54335/#comment229263>

    I actually forget whether this is style compliant. We definitely do this in other parts
of the codebase, (_e.g._, [1]), but I have occasionally been told in cases like these to put
the first argument on the next line, and to indent those lines 4 spaces. I prefer this way,
because it makes it easier to read the `!= S_OK`.
    
    I'm not opening this as an issue, but I'm still leaving this as a comment to give Joseph
the opportunity to set me straight here. :)
    
    [1] https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/io.cpp#L91-L97


- Alex Clemmer


On Dec. 8, 2016, 1:49 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, 1:49 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 COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
> 
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
> 
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354

>   3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f

> 
> Diff: https://reviews.apache.org/r/54335/diff/
> 
> 
> Testing
> -------
> 
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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