Re: [chromium-dev] Thoughts on "// NOLINT"?
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) && 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"?
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 >>> ; // <-- 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
Re: [chromium-dev] Thoughts on "// NOLINT"?
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 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 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 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"?
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 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 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 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_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"?
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. 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 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 > >> ; // <-- 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 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 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 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 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: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); #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"?
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 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"?
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 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
Re: [chromium-dev] Thoughts on "// NOLINT"?
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 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"?
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---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"?
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 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 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"?
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 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
[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
