I'd vote to keep as well.
- Dan
On Mon, Sep 19, 2016 at 2:19 PM, Adar Dembo <a...@cloudera.com> wrote:
> Personally I haven't seen much of a need for abbreviated parameter
> names. I'd vote for keeping the warning.
>
> On Mon, Sep 19, 2016 at 2:14 PM, Todd Lipcon <t...@cloudera.com> wrote:
> > 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
>
