Re: [chromium-dev] Thoughts on // NOLINT?
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 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); #else return DoWork(foo); #endif The same number of lines, but much easier to read. disagree. It's harder to read because it's not immediately obvious that some of the code overlaps. Scott's solution seems best to me. +1 Scott's solution seems best for me. The problem with the above solution is that it contains code duplication. For DoWork(foo), that might seem small, but as parameters get added, functions get renamed, etc, it's more work (and error prone) to update two locations instead of one. Opps sorry for delay in following up (I'm still tuning my filters to cope with @chromium) Scott's solution was also the first thought of, so very happy to go with that instead :-) What stopped me proposing this in the CL was I had noticed some nearby code does it via multiple returns, so I had attempted to keep consistent with the pattern:- if (!DoWork(foo)) return false; #if defined(OS_POSIX) if (!DoWork(posix_specific)) return false; #end return true; Quite agree this doesn't look so obvious though. Cheers! -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 of the Chrome NOLINTs look like the're around ifdefs, where the ifdef code sometimes mean that a comma or a semicolon goes on the line after the ifdef. We should be working to remove these anyway since the ifdefs are super ugly, and I'm not sure the NOLINT comment actually makes them worse. Some of these may not be practical or desirable to remove, though. This is exactly the case I came across recently (which maybe what inspired John to start this thread.) In essence: return DoWork(foo) #if defined(OS_POSIX) DoWork(posix_specific) #endif ; // -- Lint complains about this guy We converged on NOLINT as the solution, but I admit my ingrained instinct is to pull out the #if altogether, and try to make both calls to DoWork() be acceptable to call on all platforms, or at the very least replace the second with a wrapper its body with a null implementation on !OS_POSIX. Do we have agreed guidelines on when to use #if for portability, or which patterns to prefer to it? -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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, 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 ifdef code sometimes mean that a comma or a semicolon goes on the line after the ifdef. We should be working to remove these anyway since the ifdefs are super ugly, and I'm not sure the NOLINT comment actually makes them worse. Some of these may not be practical or desirable to remove, though. This is exactly the case I came across recently (which maybe what inspired John to start this thread.) In essence: return DoWork(foo) #if defined(OS_POSIX) DoWork(posix_specific) #endif ; // -- Lint complains about this guy We converged on NOLINT as the solution, but I admit my ingrained instinct is to pull out the #if altogether, and try to make both calls to DoWork() be acceptable to call on all platforms, or at the very least replace the second with a wrapper its body with a null implementation on !OS_POSIX. Do we have agreed guidelines on when to use #if for portability, or which patterns to prefer to it? From the code I've seen, the preference is if only one platform needs a function to be called, then we only call it using an ifdef instead of defining that empty function in all other platforms. Things like PlatformInit, PlatformDestroy are an exeception since it makes sense that you want to give each platform the capability for that. But if you look at places like PluginService or PluginProcessHost, there are things that only make sense to call on one platform. -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 prefer this: #if defined(OS_POSIX) return DoWork(foo) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. Or: bool ret = DoWork(foo); #if defined(OS_POSIX) ret = ret DoWork(posix_specific); #endif return ret; Breaking a single statement up with a macro is icky. -scott -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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) DoWork(posix_specific) #endif ; // -- Lint complains about this guy I'd prefer this: #if defined(OS_POSIX) return DoWork(foo) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. Or: bool ret = DoWork(foo); #if defined(OS_POSIX) ret = ret DoWork(posix_specific); #endif return ret; Breaking a single statement up with a macro is icky. 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. -- Jacob -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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, but compact. The Google C++ Style Guide says to avoid macros when there is a non-macro way to do the same thing. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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( DoWork(posix_specific)); where POSIX_ONLY will expand to nothing or its argument. It's ugly, but compact. The Google C++ Style Guide says to avoid macros when there is a non-macro way to do the same thing. Sanity says that too. Scott's technique is the best. If you can't figure out what's happening by glancing at the code, it's not code, it's assembly. M-A (grr) -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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. Don't do what Jacob proposed in this case: it's a net a readability loss because the pattern is unfamiliar. (Sorry, Jacob!) Mark 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) DoWork(posix_specific) #endif ; // -- Lint complains about this guy I'd prefer this: #if defined(OS_POSIX) return DoWork(foo) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. Or: bool ret = DoWork(foo); #if defined(OS_POSIX) ret = ret DoWork(posix_specific); #endif return ret; Breaking a single statement up with a macro is icky. -scott -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 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. The Google C++ Style Guide says to avoid macros when there is a non-macro way to do the same thing. Sanity says that too. Scott's technique is the best. If you can't figure out what's happening by glancing at the code, it's not code, it's assembly. I'd say: If you can't figure out what's happening by glancing at the code, it's not code, it's template metaprogramming. Assembly is easy! [Makes a face in the general direction of Herb Sutter.] LOL Were you looking at src/base/singleton.h lately? M-A -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 prefer this: #if defined(OS_POSIX) return DoWork(foo) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. disagree. It's harder to read because it's not immediately obvious that some of the code overlaps. Scott's solution seems best to me. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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) DoWork(posix_specific) #endif ; // -- Lint complains about this guy I'd prefer this: #if defined(OS_POSIX) return DoWork(foo) DoWork(posix_specific); #else return DoWork(foo); #endif The same number of lines, but much easier to read. disagree. It's harder to read because it's not immediately obvious that some of the code overlaps. Scott's solution seems best to me. +1 Scott's solution seems best for me. The problem with the above solution is that it contains code duplication. For DoWork(foo), that might seem small, but as parameters get added, functions get renamed, etc, it's more work (and error prone) to update two locations instead of one. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
[chromium-dev] Thoughts on // NOLINT?
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 consistent a bit ironic. I also find it distracting when reading the code. Am I the only one? -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 another to muddy the source to achieve that goal. grep'ing through the code, I found hundreds of NOLINTs though, so maybe we're in the minority. 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 code for tools that are supposed to make the code quality better happy/more consistent a bit ironic. I also find it distracting when reading the code. Am I the only one? -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 code for tools that are supposed to make the code quality better happy/more consistent a bit ironic. I also find it distracting when reading the code. Am I the only one? You are not. We should prefer linter errors to // NOLINT comments, because we should prefer to optimize code readability at the expense of reviewers making more judgment calls, and these comments are definitely visual noise that detracts from readability. I support an immediate s/\/\/\ NOLINT//g. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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. 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 mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 ifdefs, where the ifdef code sometimes mean that a comma or a semicolon goes on the line after the ifdef. We should be working to remove these anyway since the ifdefs are super ugly, and I'm not sure the NOLINT comment actually makes them worse. Some of these may not be practical or desirable to remove, though. So I don't think I see a big problem with the way NOLINT is used in Chrome. Looking through V8 I don't see a huge problem either. Some NOLINT calls weren't clear to me why the linter complained. I suggest that NOLINT comments be documented. In some places they already are. Then reviewers will know to argue when they see something untoward, whereas just // NOLINT isn't alwasy clear about what the problem is and whether they should complain. This will also make NOLINTs more painful to add since you have to justify why you're adding it, which will hopefully decrease its usage. Brett -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Thoughts on // NOLINT?
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 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 ifdefs, where the ifdef code sometimes mean that a comma or a semicolon goes on the line after the ifdef. We should be working to remove these anyway since the ifdefs are super ugly, and I'm not sure the NOLINT comment actually makes them worse. Some of these may not be practical or desirable to remove, though. So I don't think I see a big problem with the way NOLINT is used in Chrome. Looking through V8 I don't see a huge problem either. Some NOLINT calls weren't clear to me why the linter complained. I suggest that NOLINT comments be documented. In some places they already are. Then reviewers will know to argue when they see something untoward, whereas just // NOLINT isn't alwasy clear about what the problem is and whether they should complain. This will also make NOLINTs more painful to add since you have to justify why you're adding it, which will hopefully decrease its usage. Brett -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
