Another couple of things: Tidy-bot uses a very strict definition of lexicographic ordering, e.g. it prefers:
#include "kudu/client/value-internal.h" #include "kudu/client/value.h" Whereas we have typically preferred the reverse ordering. I'm +1 on tidy bot's way, since it allows imports to be ordered by the 'sort' command, and frees us from the shackles of manual drudgery. Tidy-bot warns on unused variables in override methods: https://gerrit.cloudera.org/#/c/2986/9/src/kudu/client/scan_predicate.cc. Arguably this is indicative of a bad interface design, but it's a little annoying. - Dan On Mon, Sep 19, 2016 at 2:32 PM, David Alves <[email protected]> wrote: > +1 to keep, abbreviating from the definition seems evil. > > -david > > On Mon, Sep 19, 2016 at 2:19 PM, Adar Dembo <[email protected]> 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 <[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 > > >
