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 >>
