----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49564/#review140516 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/stout/include/stout/elf.hpp (line 84) <https://reviews.apache.org/r/49564/#comment205925> Cleanups in a separate patch next time? 3rdparty/stout/include/stout/elf.hpp (line 122) <https://reviews.apache.org/r/49564/#comment205922> We probably don't need to say ELF here at all? The other messages seem to already omit ELF. :) Ditto above in that we should try to avoid cleanups in the same patch. 3rdparty/stout/include/stout/elf.hpp (line 138) <https://reviews.apache.org/r/49564/#comment205923> stray // here? 3rdparty/stout/include/stout/elf.hpp (line 144) <https://reviews.apache.org/r/49564/#comment205924> Can we make this const? How about using stout's Version class? ``` Result<Version> get_abi_version() { ... return Version(version[1], version[2], version[3]); } ``` 3rdparty/stout/include/stout/elf.hpp (line 159) <https://reviews.apache.org/r/49564/#comment205926> "only" suggests that there are more than 1 entries, but there can be 0 here according to the condition being checked 3rdparty/stout/include/stout/elf.hpp (lines 177 - 184) <https://reviews.apache.org/r/49564/#comment205927> Do we need to mention the patch here? src/tests/ldcache_tests.cpp (lines 66 - 69) <https://reviews.apache.org/r/49564/#comment205928> This needs to be in a separate commit because of stout/libprocess/mesos split. - Benjamin Mahler On July 2, 2016, 7:38 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49564/ > ----------------------------------------------------------- > > (Updated July 2, 2016, 7:38 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5769 > https://issues.apache.org/jira/browse/MESOS-5769 > > > Repository: mesos > > > Description > ------- > > This function returns the ABI version of the ELF file by parsing the > contents of its `.note.ABI-tag` section. This section is Linux > specific and adheres to the format described in the links below. > > https://refspecs.linuxfoundation.org/LSB_1.2.0/gLSB/noteabitag.html > http://flint.cs.yale.edu/cs422/doc/ELF_Format.pdf > > > Diffs > ----- > > 3rdparty/stout/include/stout/elf.hpp > 5382acc1c12901ce8d8256f6010f48579020fbca > src/tests/ldcache_tests.cpp b387a8b763d241075392f65256d9b39429909d8b > > Diff: https://reviews.apache.org/r/49564/diff/ > > > Testing > ------- > > `GTEST_FILTER="*Ldcache*" make -j check` > > > Thanks, > > Kevin Klues > >
