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

Reply via email to