mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 72114: Removed remaining domain socket code from the Windows build.
Date Tue, 11 Feb 2020 16:50:52 GMT

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


Ship it!




For me personally this goes a little far in ifdef'ing out code (the flags could be left in
on Windows, even though they are never added; the test could be disabled at runtime with `TEST_F_TEMP_DISABLED_ON_WINDOWS`
or sim; only the part in `slave.cpp` we seem to really need to exclude if we don't want to
`CHECK` for either the variable being set `||` windows), but it probably still fits with what
we do elsewhere.

The reason I am weary of ifdef's is that they affect code visibility which can make it harder
to work with the code (e.g., _find references_ in some IDE might not show all references anymore).
It also affects what code e.g., static analyzers can see. I believe we do too much of this
already, but it is probably okay here since we do not select for features, but for a platform
(selecting in ifdef's on features leads to a hard to control explosion of flag combinations
needed so e.g., a static analyzer can see all code).

- Benjamin Bannier


On Feb. 11, 2020, 5:38 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72114/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2020, 5:38 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> These changes are needed to get the tests to run.
> 
> 
> Diffs
> -----
> 
>   src/slave/flags.hpp 838aaee0b921b1b59d4e2b2fb69d861eec95ba56 
>   src/slave/slave.cpp 75bf59566a327a05cf7e763409b60f30e9432c86 
>   src/tests/command_executor_tests.cpp 73f80063631f1d004be307fdce77d1b405468e14 
> 
> 
> Diff: https://reviews.apache.org/r/72114/diff/1/
> 
> 
> Testing
> -------
> 
> `cmake --build . --target tests`
> `src\mesos-tests.exe`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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