----------------------------------------------------------- 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 > >
