Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10675002:
Fix thread_restrictions ifdef (Closed)
|
Created:
8 years, 6 months ago by jam Modified:
8 years, 6 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix thread_restrictions ifdef
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=144288
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... File base/threading/thread_restrictions.h (right): http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... base/threading/thread_restrictions.h:219: missing #undef
http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... File base/threading/thread_restrictions.h (right): http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... base/threading/thread_restrictions.h:219: On 2012/06/26 19:18:31, Ryan Sleevi wrote: > missing #undef they're all matched...
http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... File base/threading/thread_restrictions.h (right): http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... base/threading/thread_restrictions.h:219: On 2012/06/26 19:32:51, John Abd-El-Malek wrote: > On 2012/06/26 19:18:31, Ryan Sleevi wrote: > > missing #undef > > they're all matched... not #endif, #undef. You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never #undef'd in the header, leaving it floating around.
http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... File base/threading/thread_restrictions.h (right): http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... base/threading/thread_restrictions.h:219: On 2012/06/26 20:26:14, Ryan Sleevi wrote: > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: > > > missing #undef > > > > they're all matched... > > not #endif, #undef. > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never #undef'd in > the header, leaving it floating around. ah, I misunderstood. apart from being harmless, it's also used in the cc file
On 2012/06/26 20:28:56, jabdelmalek wrote: > http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... > File base/threading/thread_restrictions.h (right): > > http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... > base/threading/thread_restrictions.h:219: > On 2012/06/26 20:26:14, Ryan Sleevi wrote: > > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: > > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: > > > > missing #undef > > > > > > they're all matched... > > > > not #endif, #undef. > > > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never #undef'd > in > > the header, leaving it floating around. > > ah, I misunderstood. > > apart from being harmless, it's also used in the cc file The Google C++ style guide discourages such symbol polution - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro... Perhaps we're better off changing the existing instances to !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON) [I think three checks in your case in the .h, but only two checks in the other existing places].
On 2012/06/26 20:33:42, Ryan Sleevi wrote: > On 2012/06/26 20:28:56, jabdelmalek wrote: > > > http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... > > File base/threading/thread_restrictions.h (right): > > > > > http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrict... > > base/threading/thread_restrictions.h:219: > > On 2012/06/26 20:26:14, Ryan Sleevi wrote: > > > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: > > > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: > > > > > missing #undef > > > > > > > > they're all matched... > > > > > > not #endif, #undef. > > > > > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never > #undef'd > > in > > > the header, leaving it floating around. > > > > ah, I misunderstood. > > > > apart from being harmless, it's also used in the cc file > > The Google C++ style guide discourages such symbol polution - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro... I think your reading of that section is incorrect. The style guide is talking about macros that change the code, and the reason it's discouraging this is because one can't tell what the code is doing without having to look at the macro. Examples of that are things like CHECK_ALL_PARAMS_ARE_VALID(param1, param2) etc. It's not really talking about #defines, which we use all over the code (in headers) and isn't called out by the style guide. > > Perhaps we're better off changing the existing instances to !defined(NDEBUG) || > defined(DCHECK_ALWAYS_ON) [I think three checks in your case in the .h, but only > two checks in the other existing places].
On Tue, Jun 26, 2012 at 1:40 PM, <jam@chromium.org> wrote: > On 2012/06/26 20:33:42, Ryan Sleevi wrote: > >> On 2012/06/26 20:28:56, jabdelmalek wrote: >> > >> > > http://codereview.chromium.**org/10675002/diff/1/base/** > threading/thread_restrictions.**h<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h> > >> > File base/threading/thread_**restrictions.h (right): >> > >> > >> > > http://codereview.chromium.**org/10675002/diff/1/base/** > threading/thread_restrictions.**h#newcode219<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h#newcode219> > >> > base/threading/thread_**restrictions.h:219: >> > On 2012/06/26 20:26:14, Ryan Sleevi wrote: >> > > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: >> > > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: >> > > > > missing #undef >> > > > >> > > > they're all matched... >> > > >> > > not #endif, #undef. >> > > >> > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never >> #undef'd >> > in >> > > the header, leaving it floating around. >> > >> > ah, I misunderstood. >> > >> > apart from being harmless, it's also used in the cc file >> > > The Google C++ style guide discourages such symbol polution - >> > > http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml?showone=**Preprocessor_Macros#**Preprocessor_Macros<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preprocessor_Macros#Preprocessor_Macros> > > I think your reading of that section is incorrect. The style guide is > talking > about macros that change the code, and the reason it's discouraging this is > because one can't tell what the code is doing without having to look at the > macro. Examples of that are things like CHECK_ALL_PARAMS_ARE_VALID(** > param1, > param2) etc. It's not really talking about #defines, which we use all over > the > code (in headers) and isn't called out by the style guide. I'm not sure I'd agree on that - it gives the specific examples of using macros to abbreviate long variable name, storing a constant, or conditionally compile code. This context very clearly seems to indicate simple '#defines' (eg: not just #defining functions) I realize this isn't always adhered to (eg: the IPC message map macros), but I think the argument that it applies to variables, and that they should be #undef'd after using, is consistent with the style guide about points such as IWYU and header ordering - headers (and their orders) shouldn't have side-effects, which this one does. It's certainly possible I'm being overly strict in interpreting the style guide, and perhaps someone more familiar should weigh in with an opinion? > > > > Perhaps we're better off changing the existing instances to >> !defined(NDEBUG) >> > || > >> defined(DCHECK_ALWAYS_ON) [I think three checks in your case in the .h, >> but >> > only > >> two checks in the other existing places]. >> > > > > http://codereview.chromium.**org/10675002/<http://codereview.chromium.org/106... >
On Tue, Jun 26, 2012 at 1:51 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > On Tue, Jun 26, 2012 at 1:40 PM, <jam@chromium.org> wrote: > >> On 2012/06/26 20:33:42, Ryan Sleevi wrote: >> >>> On 2012/06/26 20:28:56, jabdelmalek wrote: >>> > >>> >> >> http://codereview.chromium.**org/10675002/diff/1/base/** >> threading/thread_restrictions.**h<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h> >> >>> > File base/threading/thread_**restrictions.h (right): >>> > >>> > >>> >> >> http://codereview.chromium.**org/10675002/diff/1/base/** >> threading/thread_restrictions.**h#newcode219<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h#newcode219> >> >>> > base/threading/thread_**restrictions.h:219: >>> > On 2012/06/26 20:26:14, Ryan Sleevi wrote: >>> > > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: >>> > > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: >>> > > > > missing #undef >>> > > > >>> > > > they're all matched... >>> > > >>> > > not #endif, #undef. >>> > > >>> > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never >>> #undef'd >>> > in >>> > > the header, leaving it floating around. >>> > >>> > ah, I misunderstood. >>> > >>> > apart from being harmless, it's also used in the cc file >>> >> >> The Google C++ style guide discourages such symbol polution - >>> >> >> http://google-styleguide.**googlecode.com/svn/trunk/** >> cppguide.xml?showone=**Preprocessor_Macros#**Preprocessor_Macros<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preprocessor_Macros#Preprocessor_Macros> >> >> I think your reading of that section is incorrect. The style guide is >> talking >> about macros that change the code, and the reason it's discouraging this >> is >> because one can't tell what the code is doing without having to look at >> the >> macro. Examples of that are things like CHECK_ALL_PARAMS_ARE_VALID(** >> param1, >> param2) etc. It's not really talking about #defines, which we use all >> over the >> code (in headers) and isn't called out by the style guide. > > > I'm not sure I'd agree on that - it gives the specific examples of using > macros to abbreviate long variable name, storing a constant, or > conditionally compile code. This context very clearly seems to indicate > simple '#defines' (eg: not just #defining functions) > > I realize this isn't always adhered to (eg: the IPC message map macros), > but I think the argument that it applies to variables, and that they should > be #undef'd after using, is consistent with the style guide about points > such as IWYU and header ordering - headers (and their orders) shouldn't > have side-effects, which this one does. > > It's certainly possible I'm being overly strict in interpreting the style > guide, and perhaps someone more familiar should weigh in with an opinion? > This isn't convincing, I still think your interpretation of the docs is missing what it's really against (the example I gave). I don't think going back and forth is a good use of time. This pattern of using #defines in headers is something that is used all over the code, from OS_FOO to feature disabling (defined through gyp, but same thing). > > >> >> >> >> Perhaps we're better off changing the existing instances to >>> !defined(NDEBUG) >>> >> || >> >>> defined(DCHECK_ALWAYS_ON) [I think three checks in your case in the .h, >>> but >>> >> only >> >>> two checks in the other existing places]. >>> >> >> >> >> http://codereview.chromium.**org/10675002/<http://codereview.chromium.org/106... >> > >
It seems like the ENABLE_THREAD_RESTRICTIONS is a "feature flag" that's defined in a corresponding header file, which seems OK to me. The style guide seems to be talking about macros that generate code for convenience sake.
On Tue, Jun 26, 2012 at 2:01 PM, John Abd-El-Malek <jabdelmalek@google.com>wrote: > > > > On Tue, Jun 26, 2012 at 1:51 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > >> On Tue, Jun 26, 2012 at 1:40 PM, <jam@chromium.org> wrote: >> >>> On 2012/06/26 20:33:42, Ryan Sleevi wrote: >>> >>>> On 2012/06/26 20:28:56, jabdelmalek wrote: >>>> > >>>> >>> >>> http://codereview.chromium.**org/10675002/diff/1/base/** >>> threading/thread_restrictions.**h<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h> >>> >>>> > File base/threading/thread_**restrictions.h (right): >>>> > >>>> > >>>> >>> >>> http://codereview.chromium.**org/10675002/diff/1/base/** >>> threading/thread_restrictions.**h#newcode219<http://codereview.chromium.org/10675002/diff/1/base/threading/thread_restrictions.h#newcode219> >>> >>>> > base/threading/thread_**restrictions.h:219: >>>> > On 2012/06/26 20:26:14, Ryan Sleevi wrote: >>>> > > On 2012/06/26 19:32:51, John Abd-El-Malek wrote: >>>> > > > On 2012/06/26 19:18:31, Ryan Sleevi wrote: >>>> > > > > missing #undef >>>> > > > >>>> > > > they're all matched... >>>> > > >>>> > > not #endif, #undef. >>>> > > >>>> > > You #define ENABLE_THREAD_RESTRICTIONS on line 13/15, but it's never >>>> #undef'd >>>> > in >>>> > > the header, leaving it floating around. >>>> > >>>> > ah, I misunderstood. >>>> > >>>> > apart from being harmless, it's also used in the cc file >>>> >>> >>> The Google C++ style guide discourages such symbol polution - >>>> >>> >>> http://google-styleguide.**googlecode.com/svn/trunk/** >>> cppguide.xml?showone=**Preprocessor_Macros#**Preprocessor_Macros<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preprocessor_Macros#Preprocessor_Macros> >>> >>> I think your reading of that section is incorrect. The style guide is >>> talking >>> about macros that change the code, and the reason it's discouraging this >>> is >>> because one can't tell what the code is doing without having to look at >>> the >>> macro. Examples of that are things like CHECK_ALL_PARAMS_ARE_VALID(** >>> param1, >>> param2) etc. It's not really talking about #defines, which we use all >>> over the >>> code (in headers) and isn't called out by the style guide. >> >> >> I'm not sure I'd agree on that - it gives the specific examples of using >> macros to abbreviate long variable name, storing a constant, or >> conditionally compile code. This context very clearly seems to indicate >> simple '#defines' (eg: not just #defining functions) >> >> I realize this isn't always adhered to (eg: the IPC message map macros), >> but I think the argument that it applies to variables, and that they should >> be #undef'd after using, is consistent with the style guide about points >> such as IWYU and header ordering - headers (and their orders) shouldn't >> have side-effects, which this one does. >> >> It's certainly possible I'm being overly strict in interpreting the style >> guide, and perhaps someone more familiar should weigh in with an opinion? >> > > This isn't convincing, I still think your interpretation of the docs is > missing what it's really against (the example I gave). I don't think going > back and forth is a good use of time. This pattern of using #defines in > headers is something that is used all over the code, from OS_FOO to feature > disabling (defined through gyp, but same thing). > I think your characterization of "All over" is not accurate, nor do I think it reflects what you're doing here. Yes, there are a few headers within base that provide certain, cross-platform/cross-toolchain #defines (normalizing compiler-provided defines such as X86 vs ARM vs X64, the C98 vs C99 defines of basictypes.h, the OS_FOO/USE_FOO defines of build_config.h), but those are part of the toolchain flattening. That is, they're typically uniformly provided by our compiler tools, we're just normalizing them to a common naming scheme. The use of 'defines' in GYP files are not the same as what's being done here, because they apply to all files within a target (set of targets), regardless of header including or ordering (which is sort of my point here), and they can be easily set to propogate to targets if dependents may matter (eg: export_dependent_settings). There's a very real and inherent danger when defining flags in a header file, and letting them propagate. Real issues that I've seen in the past, which leads me to interpret the style guide as such: - Dependent code #includes some header that #defines a macro, and then itself conditionally compiles based on that macro. Some later point, someone does IWYU optimization, remove the #header defining that macro (since it's not "obvious" the necessity of it), and the result is the code now compiles differently based on header includes. - A macro that is conditionally gated upon some condition in a header file, but the conditions for that macro change between different targets. Different targets thus have different views of object layout, leading to odd or hard to find crashes. We typically address these by making sure the #defines are either in build/common.gypi (and thus applied to all targets equally and uniformly) or are defined within the gyp file and 'export_dependent_settings' as appropriate. Examples within Chromium of where this has been abused/misused is things like using defined(UNIT_TESTS) in header files (since UNIT_TESTS is only defined within the actual test target, by virtue of gtest's export_dependent_settings). I can also point to issues in corp code or in general open source of similar effect. Using the non-feature-flag #define approach of "(!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)", or defining a flag in build/common.gypi (as the thread I linked to proposed, but never implemented), seems more correct then leaking the #define past the header. We can say these are "clear abuses of the language" or "obvious errors" when they occur, but I don't think that's always the case, which is why I take the view of the style guide that I do. Considering that it goes to the point of discouraging excessive templates, default values, implicit constructors, etc, all of which are "obvious if you know what you're doing", the subtlety of 'leaked macros' seems very much in scope. > >> >>> >>> >>> >>> Perhaps we're better off changing the existing instances to >>>> !defined(NDEBUG) >>>> >>> || >>> >>>> defined(DCHECK_ALWAYS_ON) [I think three checks in your case in the .h, >>>> but >>>> >>> only >>> >>>> two checks in the other existing places]. >>>> >>> >>> >>> >>> http://codereview.chromium.**org/10675002/<http://codereview.chromium.org/106... >>> >> >> >
And the perf argument: MSVC and GCC, at least through 2008/4.4, cannot cache header pre-processing if #defines appear within them that are not #undef'd. That is, it prevents the implicit '#pragma once' style of 'process once and splat into the file', due to the fact that the #define tree may have changed.
It doesn't seem like this patch is changing anything in this respect, and I don't perceive this instance to be a significant problem one way or the other (I'm OK with either approach).
LGTM. I don't think this patch is worth having a big disagreement on, especially when nothing about the #defines are changing now. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
