On Fri, Sep 23, 2016 at 10:57 AM, Dan Burkert <d...@cloudera.com> wrote:
> 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. > I think the intention is that you should comment out the parameter name, like: void Foo(int /*bar*/) { ... } This is suggested by the Google guide: https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions "Unused parameters that might not be obvious should comment out the variable name in the function definition" -Todd On Mon, Sep 19, 2016 at 2:32 PM, David Alves <davidral...@gmail.com> wrote: > > > +1 to keep, abbreviating from the definition seems evil. > > > > -david > > > > 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 > > > > > > -- Todd Lipcon Software Engineer, Cloudera
