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

2009-12-14 Thread Jonathan Dixon
2009/12/10 John Abd-El-Malek j...@chromium.org On Thu, Dec 10, 2009 at 1:22 PM, Evan Stade est...@chromium.org wrote: On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting pkast...@google.comwrote: On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.orgwrote: In essence: return

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

2009-12-10 Thread Jonathan Dixon
2009/12/10 Brett Wilson bre...@chromium.org On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek j...@chromium.org 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

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

2009-12-10 Thread Peter Kasting
On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.org 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)

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 j...@chromium.org wrote: 2009/12/10 Brett Wilson bre...@chromium.org On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek j...@chromium.org wrote: btw I searched the code, almost all the instances are in code from different repositories,

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

2009-12-10 Thread Scott Hess
On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting pkast...@google.com wrote: On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.org wrote: In essence: return DoWork(foo) #if defined(OS_POSIX)      DoWork(posix_specific) #endif     ;  // -- Lint complains about this guy I'd

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 pkast...@google.com wrote: On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.org wrote: In essence: return DoWork(foo) #if defined(OS_POSIX)     

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

2009-12-10 Thread Peter Kasting
On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson ja...@mandelson.orgwrote: 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,

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

2009-12-10 Thread Marc-Antoine Ruel
On Thu, Dec 10, 2009 at 2:24 PM, Peter Kasting pkast...@google.com wrote: On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson ja...@mandelson.org wrote: If something extra in an expression is a common case, I've sometimes seen it done like: return DoWork(foo) POSIX_ONLY(

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.

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 mar...@chromium.orgwrote: On Thu, Dec 10, 2009 at 2:24 PM, Peter Kasting pkast...@google.com wrote: On Thu, Dec 10, 2009 at 11:20 AM, Jacob Mandelson ja...@mandelson.org

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

2009-12-10 Thread Evan Stade
On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting pkast...@google.com wrote: On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.orgwrote: In essence: return DoWork(foo) #if defined(OS_POSIX) DoWork(posix_specific) #endif ; // -- Lint complains about this guy I'd

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 est...@chromium.org wrote: On Thu, Dec 10, 2009 at 10:58 AM, Peter Kasting pkast...@google.comwrote: On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon j...@chromium.orgwrote: In essence: return DoWork(foo) #if defined(OS_POSIX)

[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

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

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 j...@chromium.org 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

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

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 est...@chromium.org wrote: I didn't even know that I could disable the linter like that.

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 j...@chromium.org 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

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 bre...@chromium.org wrote: On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek