How do people feel about this warning?

warning: function 'kudu::InsertLoadgen::InserterThread' has a definition
with different parameter names
[readability-inconsistent-declaration-parameter-name]
void InserterThread(Generator::Mode gen_mode, int64_t seed,
^
gen_seed
src/kudu/tools/insert-generated-rows.cc:307:21: note: the definition seen
here
void InsertLoadgen::InserterThread(Generator::Mode gen_mode, int64_t
gen_seed,
^
src/kudu/tools/insert-generated-rows.cc:234:8: note: differing parameters
are named here: ('seed'), in definition: ('gen_seed')
void InserterThread(Generator::Mode gen_mode, int64_t seed,


Is it a reasonable one to keep, or should I disable it? On the one hand, it
might catch a bug where the order of parameters is swapped between the
definition and declaration, or where you forgot to update the name when
changing the definition. On the other hand, sometimes it can make sense to
have an "abbreviated" parameter name in the function definition.

-Todd

On Fri, Sep 16, 2016 at 10:42 AM, Adar Dembo <a...@cloudera.com> wrote:

> OK, let's keep it then. I changed the comment in question to be
> "TODO(KUDU-1537)...".
>
> On Thu, Sep 15, 2016 at 8:18 PM, Jake Farrell <jfarr...@apache.org> wrote:
> > we use "TODO(bug id or committer id): msg" as the format in other
> projects
> > and that seems to be enough breadcrumb in most cases
> >
> > -Jake
> >
> > On Thu, Sep 15, 2016 at 11:06 PM, Todd Lipcon <t...@cloudera.com> wrote:
> >
> >> On Thu, Sep 15, 2016 at 6:50 PM, Adar Dembo <a...@cloudera.com> wrote:
> >>
> >> > In https://gerrit.cloudera.org/#/c/4435/, Tidy Bot said:
> >> >
> >> > Line 209:       // TODO: Should be fixed with Exactly Once semantics,
> >> > see KUDU-1537.
> >> > warning: missing username/bug in TODO [google-readability-todo]
> >> >       // TODO: Should be fixed with Exactly Once semantics, see
> >> KUDU-1537.
> >> >       ^
> >> >       // TODO(unknown): Should be fixed with Exactly Once semantics,
> >> > see KUDU-1537.
> >> >
> >> > This doesn't look like the kind of style change we want, right?
> >> > Historically we don't annotate our TODOs with names.
> >> >
> >> > Or should I reformat it as "TODO(KUDU-1537)..." ?
> >> >
> >>
> >> Yea, I think TODO(bug#) is a good policy to try to have moving forward,
> but
> >> I don't think we have to be religious about it. I can turn off this tidy
> >> check if we think it's not worth it with a codebase of our size.
> >>
> >> This guideline comes from Google (
> >> https://google.github.io/styleguide/cppguide.html#TODO_Comments) where
> >> they
> >> say:
> >>
> >> TODOs should include the string TODO in all caps, followed by the name,
> >> e-mail address, bug ID, or other identifier of the person or issue with
> the
> >> best context about the problem referenced by the TODO. The main purpose
> is
> >> to have a consistent TODO that can be searched to find out how to get
> more
> >> details upon request. A TODO is not a commitment that the person
> referenced
> >> will fix the problem. Thus when you create a TODO with a name, it is
> almost
> >> always your name that is given.
> >> It's sort of nice to leave a breadcrumb, but it's also not too hard for
> >> someone to 'git blame' and figure out who added it, so I could go either
> >> way.
> >>
> >> -Todd
> >>
> >>
> >> >
> >> >
> >> > On Wed, Sep 14, 2016 at 10:04 PM, Todd Lipcon <t...@cloudera.com>
> wrote:
> >> > > Hey folks,
> >> > >
> >> > > I've set up a jenkins job and gerrit trigger to run clang-tidy-diff
> on
> >> > any
> >> > > patches that are uploaded. It should be set up now so as not to
> vote +1
> >> > or
> >> > > -1, but just to produce comments. For an example of the type of
> >> warnings
> >> > it
> >> > > generates, check out:
> >> > > https://gerrit.cloudera.org/#/c/4409/4/src/kudu/consensus/
> >> > raft_consensus_state.h
> >> > >
> >> > > If you see any checks that you think are false positives, feel free
> to
> >> > ping
> >> > > me and I can either disable those checks entirely, or see if there's
> >> some
> >> > > configuration we can make to better match our own guidelines.
> >> > >
> >> > > Hopefully this turns out to be a useful bit of "automatic code
> review"
> >> so
> >> > > that we can focus our review efforts less on mechanical checks and
> more
> >> > on
> >> > > things requiring human judgment :) If it turns out to be more of an
> >> > > annoyance than a help, we can always remove it or really dial back
> to
> >> > only
> >> > > the most important warnings.
> >> > >
> >> > > Also worth noting that these checks are not that complicated to
> write,
> >> so
> >> > > if we see that there are some Kudu-specific ones worth
> implementing, we
> >> > can
> >> > > easily do so.
> >> > >
> >> > > -Todd
> >> > > --
> >> > > Todd Lipcon
> >> > > Software Engineer, Cloudera
> >> >
> >>
> >>
> >>
> >> --
> >> Todd Lipcon
> >> Software Engineer, Cloudera
> >>
>



-- 
Todd Lipcon
Software Engineer, Cloudera

Reply via email to