lkml.org 
[lkml]   [2013]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [GIT PULL 00/21] perf/core improvements and fixes

* Ingo Molnar <mingo@kernel.org> wrote:

>
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> > -void dso__set_short_name(struct dso *dso, const char *name)
> > +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> > {
> > if (name == NULL)
> > return;
> > + if (dso->sname_alloc)
> > + free((char *)dso->short_name);
> > + dso->sname_alloc = sname_alloc;
>
> Calling the function option the same as the field name is asking for
> trouble - I'd suggest 'new_sname_alloc' for the parameter, or so.
>
> And I'd also remove the 'const' from struct dso::short_name, it
> probably does not help code generation, because 'dso' is passed in as
> const in all the non-lifetime methods anyway.
>
> That way the cast can be dropped from the free().
>
> Similar problems exist with the usage of 'short_name' - it overloads
> the field name which makes it somewhat confusing, and it's also
> sometimes inconsistently named, such as 'name' in
> dso__set_short_name().
>
> Ditto for 'long_name' handling.
>
> Also, the 'sname_alloc' name sucks, it does not make it obvious that
> it's related to 'short_name', hiding its true significance (and hiding
> the broken life time handling of the flag/pointer combo). I'd rename
> it to something more descriptive, like ->short_name_allocated - or I'd
> rename everything to 'sname'/'lname' naming for short/long names.
>
> Every time one runs into a crash like this it's a canary signal that
> cleanliness principles need hardening.

More observations about util/dso.c:

- dso__binary_type_file() should probably pass in 'const struct dso'

- dso__binary_type_file()'s filename string parameter should be named
'filename', not 'file' ...

- build_id__sprintf() looks fragile: every single use of it appears
to follow this pattern:

build_id__sprintf(x, sizeof(x), ...)

this could be simplified (and eliminating the possibility to typo a
bug) by changing the function to __build_id__snprintf() and adding
a build_id__sprintf() wrapper macro around it:

build_id__sprintf(x, ...)

that generates the size itself.

- dso__binary_type_file() is a method without a verb, so it's unclear
what it does. It probably wants to be renamed to
dso__set_binary_type_file() or so?

- dso_cache__find() probably wants to pass in a const rb_root.

- 'struct dso *pos' should probably be named 'struct dso *dso_pos' or
so - 'pos' is frequently used for integer variable names so its use
for an object iterator feels confusing.

- the 'head' argument of dsos__find() wants to be constified too I
guess

Thanks,

Ingo


\
 
 \ /
  Last update: 2013-12-10 14:01    [W:0.080 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site