mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Hopcroft" <m...@hopcroft.org>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Wed, 04 Nov 2015 18:08:29 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 26)
<https://reviews.apache.org/r/39803/#comment163470>

    Good comment!



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 32)
<https://reviews.apache.org/r/39803/#comment163473>

    I wonder if there is any good way of protecting ourselves from breaking changes associated
with _REPARSE_DATA_BUFFER. Perhaps you should consider setting WINVER in windows.hpp. Then
you could add a static_assert on the value of WINVER in this header.
    
    http://stackoverflow.com/questions/10112051/c-compile-time-macros-to-detect-windows-os



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 33)
<https://reviews.apache.org/r/39803/#comment163506>

    Consider adding one-line comments for each field.
    
    https://msdn.microsoft.com/en-us/library/cc232007.aspx



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 45)
<https://reviews.apache.org/r/39803/#comment163507>

    One line comment for this field is important since the length is in bytes, not WCHARS.
Same for line 55.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 49)
<https://reviews.apache.org/r/39803/#comment163477>

    Are you sure these buffers of size 1 are correct? I'm assuming that the remainder of the
buffer follows the _REPARSE_DATA_BUFFER in the block of memory.
    
    I would add a comment explaining this.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 51)
<https://reviews.apache.org/r/39803/#comment163478>

    Recommend putting a blank line before this comment to that it is absolutely clear which
struct it refers to.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 71)
<https://reviews.apache.org/r/39803/#comment163494>

    Who actually uses this struct. The reason I ask is that it has wstrings inside instead
of strings. Will the user be forced to do the conversion? Perhaps you should be doing the
conversion for them here.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 73)
<https://reviews.apache.org/r/39803/#comment163475>

    Be aware the std::wstring can throw exceptions. If stout has a strong no-exception guarantee,
you should review the code paths to make sure that this exception is caught somewhere inside
of stout.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 75)
<https://reviews.apache.org/r/39803/#comment163495>

    If this struct is part of the API, it should have better documentation. One cannot know
what flags is without reading the implementation in this file.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 84)
<https://reviews.apache.org/r/39803/#comment163481>

    Should check for return value of  INVALID_FILE_ATTRIBUTES or add an explanatory comment
if the code on line 85 correctly handles this case.
    
    In general, your logic should not make any assumptions on what INVALID_FILE_ATTRIBUTES
actually equals.
    
    ------
    Oh - I see - you check for INVALID_FILE_ATTRIBUTES on line 89. In general line 85 is bad
form because reparseBitSet is only valid when attributes != INVALID_FILE_ATTRIBUTES. Recommend
writing code so that reparseBitSet is valid during its entire lifetime.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 87)
<https://reviews.apache.org/r/39803/#comment163497>

    And get rid of this comment. It assumes that INVALID_FILE_ATTRIBUTES == -1, which may
not always be true. This comment shouldn't be necessary if you rework the logic per my comment
on line 84.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 104)
<https://reviews.apache.org/r/39803/#comment163501>

    Divide by sizeof(WCHAR). Same goes for lines 106, 111, and 113.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 108)
<https://reviews.apache.org/r/39803/#comment163504>

    I prefer "data.SymbolicLinkReparseBuffer.PathBuffer + printNameStartIndex", but some might
consider this a religious or stylistic difference. My opinion is that you shouldn't dereference
the pointer ([]) if you are then going to take its address.
    
    Same goes for line 115.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 145)
<https://reviews.apache.org/r/39803/#comment163508>

    "or a file" not "of a file"



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 147)
<https://reviews.apache.org/r/39803/#comment163509>

    Could you track down a more definitive source to quote?
    
    Also, perhaps you could get Microsoft to create a work item for updating MSDN.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 152)
<https://reviews.apache.org/r/39803/#comment163483>

    The ternary operator has precedence that is sometimes confusing to some programmers. Recommend
using parens to make grouping explicit.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 165)
<https://reviews.apache.org/r/39803/#comment163484>

    Finish this.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 168)
<https://reviews.apache.org/r/39803/#comment163486>

    Consider using some sort of RAII auto-handle that closes when it goes out of scope.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 199)
<https://reviews.apache.org/r/39803/#comment163488>

    Recommend using RAII pattern (e.g. std::unique_ptr) to eliminate leaking of reparsePointData
in the presense of exceptions or logic errors.
    
    Since this is C++ code, recommend using new [] instead of malloc/free.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 201)
<https://reviews.apache.org/r/39803/#comment163490>

    Consider swapping lines 199 and 201. Make allocation take reparsePointDataSize as a parameter.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 202)
<https://reviews.apache.org/r/39803/#comment163491>

    Consider naming this "ignored" instead of "junk"



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 218)
<https://reviews.apache.org/r/39803/#comment163493>

    Again, it is bad form to have the free() in two places (here and line 223). Use std::unique_ptr.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 219)
<https://reviews.apache.org/r/39803/#comment163492>

    Implement this.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 125)
<https://reviews.apache.org/r/39803/#comment163463>

    It is probably good practice to include information about the site of the error. First
idea is to just adopt a convention of prefixing each method with "functionName:". Better idea
is to modify error.hpp so that ErrnoError() becomes a macro that prepends the file name and
line number.
    
    This comment applies to all of your functions.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 128)
<https://reviews.apache.org/r/39803/#comment163464>

    Might want to static_assert that _USE_32BIT_TIME_T is not defined. Probably sound advice
for _UNICODE and _MBCS as well. Perhaps these static asserts should go in windows.hpp? If
you put them there, add a comment saying why the asserts are necessary (e.g. "Implementation
of mtime() assumes ...")



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 152)
<https://reviews.apache.org/r/39803/#comment163467>

    Have you checked all of the return types to ensure they are what you think they are? Windows
has a lot of #ifdefs for these types and it is possible that an unexpected cast could cause
problems. I would look at the definition of each field type in _stat and add static asserts
where necessary to ensure the type is what you think it is. Another possibility is that your
functions return types like "unsigned int" and perform the casting from s.st_dev directly
in your function.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 164)
<https://reviews.apache.org/r/39803/#comment163468>

    According to the Windows documentation, "The inode, and therefore st_ino, has no meaning
in the FAT, HPFS, or NTFS file systems." Will this be a problem?
    
    https://msdn.microsoft.com/en-us/library/14h5k7ff.aspx


- Michael Hopcroft


On Nov. 1, 2015, 1:31 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2015, 1:31 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van Remoortere,
and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implemented stout/os/stat.hpp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 67b142bbc2d80f40a1e893cc5813d58dd2aa8381

>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 675b2e712358a55b3580026936890eaf80e5af71

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 1a7037d64afeedc340258c92067e95d1d3caa027

> 
> Diff: https://reviews.apache.org/r/39803/diff/
> 
> 
> Testing
> -------
> 
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


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