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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> > >> >> On Thu, Sep 15, 2016 at 6:50 PM, Adar Dembo <[email protected]> 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 <[email protected]> >> 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
