Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-14 Thread Jonathan Dixon
2009/12/10 John Abd-El-Malek > > On Thu, Dec 10, 2009 at 1:22 PM, Evan Stade wrote: > >> On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting wrote: >> >>> On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: >>> In essence: return DoWork(&foo) #if defined(OS_POSIX) &&

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread John Abd-El-Malek
On Thu, Dec 10, 2009 at 1:22 PM, Evan Stade wrote: > On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting wrote: > >> On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: >> >>> In essence: >>> >>> return DoWork(&foo) >>> #if defined(OS_POSIX) >>> && DoWork(&posix_specific) >>> #endif >>> ;

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Evan Stade
On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting wrote: > On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: > >> In essence: >> >> return DoWork(&foo) >> #if defined(OS_POSIX) >> && DoWork(&posix_specific) >> #endif >> ; // <-- Lint complains about this guy >> > > I'd prefer this: >

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Marc-Antoine Ruel
On Thu, Dec 10, 2009 at 3:02 PM, Shall be Unnamed <@google.com> wrote: > On Thu, Dec 10, 2009 at 2:29 PM, Marc-Antoine Ruel wrote: > >> On Thu, Dec 10, 2009 at 2:24 PM, Peter Kasting >> wrote: >> >> On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson >>> wrote: >>> If something extra in an

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Mark Mentovai
There are cases where you'll want to flout the linter, but this isn't one of them. Scott and Peter have both posted viable workarounds that don't hamper readability (and in fact improve it relative to the snippet Jonathan is asking about.) Personally, I prefer Scott's, but Peter's is good too. D

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Marc-Antoine Ruel
On Thu, Dec 10, 2009 at 2:24 PM, Peter Kasting wrote: > On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson > wrote: > >> If something extra in an expression is a common case, I've sometimes >> seen it done like: >>return DoWork(&foo) POSIX_ONLY(&& DoWork(&posix_specific)); >> where POSIX_ONL

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Peter Kasting
On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson wrote: > If something extra in an expression is a common case, I've sometimes > seen it done like: >return DoWork(&foo) POSIX_ONLY(&& DoWork(&posix_specific)); > where POSIX_ONLY will expand to nothing or its argument. > It's ugly, but compact.

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Jacob Mandelson
On Thu, Dec 10, 2009 at 11:14:32AM -0800, Scott Hess wrote: > On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting wrote: > > On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: > >> In essence: > >> return DoWork(&foo) > >> #if defined(OS_POSIX) > >>     && DoWork(&posix_specific) > >> #endif > >

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Scott Hess
On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting wrote: > On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: >> In essence: >> return DoWork(&foo) >> #if defined(OS_POSIX) >>     && DoWork(&posix_specific) >> #endif >>     ;  // <-- Lint complains about this guy > > I'd prefer this: > #if def

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread John Abd-El-Malek
On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: > > > 2009/12/10 Brett Wilson > > On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek >> wrote: >> > btw I searched the code, almost all the instances are in code from >> different >> > repositories, like v8, gtest, gmock. I counted only 17

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Peter Kasting
On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon wrote: > In essence: > > return DoWork(&foo) > #if defined(OS_POSIX) > && DoWork(&posix_specific) > #endif > ; // <-- Lint complains about this guy > I'd prefer this: #if defined(OS_POSIX) return DoWork(&foo) && DoWork(&posix_specific)

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-10 Thread Jonathan Dixon
2009/12/10 Brett Wilson > On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek > wrote: > > btw I searched the code, almost all the instances are in code from > different > > repositories, like v8, gtest, gmock. I counted only 17 instances in > > Chrome's code. > > > Most of the Chrome NOLINTs loo

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread Elliot Glaysher (Chromium)
If there are consistent patterns of NOLINT usage, I can suppress the whole error class. Also, the linter is only automatically run on chrome/ and app/, IIRC. -- Elliot On Wed, Dec 9, 2009 at 4:38 PM, Brett Wilson wrote: > On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek wrote: >> btw I search

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread Brett Wilson
On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek wrote: > btw I searched the code, almost all the instances are in code from different > repositories, like v8, gtest, gmock.  I counted only 17 instances in > Chrome's code. Most of the Chrome NOLINTs look like the're around ifdefs, where the ifd

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread John Abd-El-Malek
btw I searched the code, almost all the instances are in code from different repositories, like v8, gtest, gmock. I counted only 17 instances in Chrome's code. On Wed, Dec 9, 2009 at 4:08 PM, Evan Stade wrote: > I didn't even know that I could disable the linter like that. Good to > know---doze

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread Evan Stade
I didn't even know that I could disable the linter like that. Good to know---dozens more NOLINTs coming up! Jokes aside, I agree the linter seems a little draconian, especially as it seems to apply to all code in the files you touch, not just your changes. -- Evan Stade -- Chromium Developers m

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread Peter Kasting
On Wed, Dec 9, 2009 at 3:48 PM, John Abd-El-Malek wrote: > Lately I've been seeing more and more // NOLINT added to the code. It's > great that people are running lint to make sure that they're following the > guidelines, but I personally find adding comments or gibberish to our code > for tools

Re: [chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread James Hawkins
Agreed. There are certain situations where conforming to lint expectations leads to messier code. I just checked in a CL that contains a section of lines longer than 80 cols. Trying to wrap these lines would make the definitions unreadable. It's one thing to have lint report zero errors; it's a

[chromium-dev] Thoughts on "// NOLINT"?

2009-12-09 Thread John Abd-El-Malek
Lately I've been seeing more and more // NOLINT added to the code. It's great that people are running lint to make sure that they're following the guidelines, but I personally find adding comments or gibberish to our code for tools that are supposed to make the code quality better happy/more consi