This is an automatically generated e-mail. To reply, visit:

Fix it, then Ship it!

3rdparty/stout/configure.ac (line 154)


3rdparty/stout/include/stout/elf.hpp (lines 20 - 33)

    Looks like we need to audit these?

3rdparty/stout/include/stout/elf.hpp (line 69)

    Hm.. could we move this down towards where it is first used?

3rdparty/stout/include/stout/elf.hpp (line 71)

    Can we document what this does?

3rdparty/stout/include/stout/elf.hpp (line 80)

    We should avoid logging the caller-available information here as it leads 
to redundant logging.

3rdparty/stout/include/stout/elf.hpp (line 91)

    Ditto on excluding the path here.

3rdparty/stout/include/stout/elf.hpp (line 94)

    `s/ */* /` here and elsewhere

3rdparty/stout/include/stout/elf.hpp (lines 94 - 106)

    Hm.. could we use some unabbreviated variable names here and elsewhere? 
E.g. 'section', 'section_header', 'section_type', etc. Since the type names are 
opaque that would help the reader.

3rdparty/stout/include/stout/elf.hpp (line 104)

    Hm.. emplace here seems a bit odd since there's no need to avoid copying 
for pointer types. Let's avoid emplace for now overall since we're not doing 
any large copying here, we can optimize it later.

3rdparty/stout/include/stout/elf.hpp (line 134)

    const for these member functions?

3rdparty/stout/include/stout/elf.hpp (line 137)

    Why cast instead of using the Class enum value?

3rdparty/stout/include/stout/elf.hpp (line 138)

    Let's do an audit to match the names in the log messages: 'gelf_getclass' 
here for example.

3rdparty/stout/include/stout/elf.hpp (line 156)

    We've started to use snake_case in stout and libprocess (we used to, then 
started using camelCase, and now we've moved back), it's unfortunately 
inconsistent so I would be fine either way here. If you don't mind we can 
switch all the variables here to use snake_case.

3rdparty/stout/include/stout/elf.hpp (line 169)

    Looks like we should make this an Option to catch the case where the loop 
completes without finding a pointer.

- Benjamin Mahler

On May 18, 2016, 8:20 p.m., Kevin Klues wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> -----------------------------------------------------------
> (Updated May 18, 2016, 8:20 p.m.)
> Review request for mesos and Benjamin Mahler.
> Bugs: MESOS-5400
>     https://issues.apache.org/jira/browse/MESOS-5400
> Repository: mesos
> Description
> -------
> Right now we are able to parse ELF formatted shared libraries and
> extract their canonical SONAME and external library dependencies. In
> the future, we should add support for fully parsing an ELf file for
> easy access to all of its contents.
> The current implementation relies on libelf. We should probably remove
> this dependency in future versions (mostly since the headers for
> libelf are not installed on a standard Linux distribution by default).
> Diffs
> -----
>   3rdparty/stout/configure.ac ada1e22c72875fe9d557f07e4846128be0bcef13 
>   3rdparty/stout/include/stout/elf.hpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/47482/diff/
> Testing
> -------
> The test for this is actually in a follow-on patch for testing ld.so.cache 
> parsing. The test itself is run with:
> ```
> GTEST_FILTER="LdcacheTest.Parse" make check -j
> ```
> Thanks,
> Kevin Klues

Reply via email to