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
