Updating DCHECK() to DCHECK_GE() in base/ dir

Updating DCHECK() to DCHECK_GE() in base/ dir

BUG=58409

[KushalP, 2011-03-20 20:40:52]

Hey guys,

I'd appreciate it if someone could review my commits.

[tfarina, 2011-03-20 21:03:11]

On 2011/03/20 20:40:52, KushalP wrote:
> Hey guys,
> 
> I'd appreciate it if someone could review my commits.

It seems you forgot to select the appropriate reviewers.

[KushalP, 2011-03-21 00:11:57]

Thanks Thiago. Didn't realise I missed that.

On 2011/03/20 21:03:11, tfarina wrote:
> On 2011/03/20 20:40:52, KushalP wrote:
> > Hey guys,
> > 
> > I'd appreciate it if someone could review my commits.
> 
> It seems you forgot to select the appropriate reviewers.

[Mohamed Mansour, 2011-03-21 01:09:01]

LGTM

[KushalP, 2011-03-22 12:41:51]

I've just looked through the Mac tryserver errors and I'm not sure where I've
broken it. Any ideas or pointers?

On 2011/03/21 01:09:01, Mohamed Mansour wrote:
> LGTM

[Peter Kasting, 2011-03-24 21:05:44]

On 2011/03/22 12:41:51, KushalP wrote:
> I've just looked through the Mac tryserver errors and I'm not sure where I've
> broken it. Any ideas or pointers?

Try modifying mach_ipc_mac.h to declare, but not define, kEmptyMessageSize; move
the definition to the .mm file.  I think that should fix it.

[KushalP, 2011-03-26 12:18:44]

Okay, I've separated out the declaration and definition. However, I'm not so
sure about whether the way I've placed it in the .mm file is the best way. Would
love to hear your thoughts.

On 2011/03/24 21:05:44, Peter Kasting wrote:
> On 2011/03/22 12:41:51, KushalP wrote:
> > I've just looked through the Mac tryserver errors and I'm not sure where
I've
> > broken it. Any ideas or pointers?
> 
> Try modifying mach_ipc_mac.h to declare, but not define, kEmptyMessageSize;
move
> the definition to the .mm file.  I think that should fix it.

[Peter Kasting, 2011-03-26 19:11:47]

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.h#newcode227
base/mach_ipc_mac.h:227: static const size_t kEmptyMessageSize;
Nit: No need for comment about where it's defined (the corresponding .cc/.mm
file is the common place).

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm#newcode52
base/mach_ipc_mac.mm:52: // The size of an empty message with no data.
Nit: Remove comment (already commented in header).

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm#newcode53
base/mach_ipc_mac.mm:53: kEmptyMessageSize = sizeof(mach_msg_header_t) +
You should instead place this below "namespace base {" above, like:

namespace base {

// static
const size_t kEmptyMessageSize = ...;

//=======...

[KushalP, 2011-03-26 20:23:13]

Done and uploaded new patch set.

I'm curious, does moving kEmptyMessageSize into `namespace base` provide any
other benefits beyond making the variable accessible to other methods in the .mm
file?

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.h#newcode227
base/mach_ipc_mac.h:227: static const size_t kEmptyMessageSize;
On 2011/03/26 19:11:47, Peter Kasting wrote:
> Nit: No need for comment about where it's defined (the corresponding .cc/.mm
> file is the common place).

Done.

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm#newcode52
base/mach_ipc_mac.mm:52: // The size of an empty message with no data.
On 2011/03/26 19:11:47, Peter Kasting wrote:
> Nit: Remove comment (already commented in header).

Done.

http://codereview.chromium.org/6688056/diff/8001/base/mach_ipc_mac.mm#newcode53
base/mach_ipc_mac.mm:53: kEmptyMessageSize = sizeof(mach_msg_header_t) +
On 2011/03/26 19:11:47, Peter Kasting wrote:
> You should instead place this below "namespace base {" above, like:
> 
> namespace base {
> 
> // static
> const size_t kEmptyMessageSize = ...;
> 
> //=======...

Done.

[Peter Kasting, 2011-03-26 23:48:24]

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode223
base/mach_ipc_mac.h:223: // kEmptyMessageSize needs to have the definition of
MachMessageData before
Nit: This comment can now be removed and the constant be moved up to the
appropriate place in the main "public" block.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode226
base/mach_ipc_mac.h:226: // The size of an empty message
Nit: Heck, let's just remove this comment, it says nothing more than the
variable name.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.mm#newcode15
base/mach_ipc_mac.mm:15: const size_t kEmptyMessageSize =
sizeof(mach_msg_header_t) +
Sorry, I meant to say "MachSendMessage::kEmptyMessageSize", you need to
explicitly give the classname.

[KushalP, 2011-03-27 00:25:15]

Updated.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode223
base/mach_ipc_mac.h:223: // kEmptyMessageSize needs to have the definition of
MachMessageData before
On 2011/03/26 23:48:24, Peter Kasting wrote:
> Nit: This comment can now be removed and the constant be moved up to the
> appropriate place in the main "public" block.

Done.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode226
base/mach_ipc_mac.h:226: // The size of an empty message
On 2011/03/26 23:48:24, Peter Kasting wrote:
> Nit: Heck, let's just remove this comment, it says nothing more than the
> variable name.

Done.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.mm#newcode15
base/mach_ipc_mac.mm:15: const size_t kEmptyMessageSize =
sizeof(mach_msg_header_t) +
On 2011/03/26 23:48:24, Peter Kasting wrote:
> Sorry, I meant to say "MachSendMessage::kEmptyMessageSize", you need to
> explicitly give the classname.

Done.

[Peter Kasting, 2011-03-27 05:56:09]

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode223
base/mach_ipc_mac.h:223: // kEmptyMessageSize needs to have the definition of
MachMessageData before
On 2011/03/27 00:25:15, KushalP wrote:
> On 2011/03/26 23:48:24, Peter Kasting wrote:
> > Nit: This comment can now be removed and the constant be moved up to the
> > appropriate place in the main "public" block.
> 
> Done.

No, it's not.  You removed the comment but didn't move the constant.

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.h#newcode212
base/mach_ipc_mac.h:212: protected:
Nit: While here, kill this extra "protected"

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.mm#newcode16
base/mach_ipc_mac.mm:16: sizeof(mach_msg_body_t) +
Nit: This indenting is strange.  Indent 4 and pull the third arg onto the same
line, please.

[KushalP, 2011-03-27 10:17:16]

Everything should now be in order.

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/13001/base/mach_ipc_mac.h#newcode223
base/mach_ipc_mac.h:223: // kEmptyMessageSize needs to have the definition of
MachMessageData before
On 2011/03/27 05:56:09, Peter Kasting wrote:
> On 2011/03/27 00:25:15, KushalP wrote:
> > On 2011/03/26 23:48:24, Peter Kasting wrote:
> > > Nit: This comment can now be removed and the constant be moved up to the
> > > appropriate place in the main "public" block.
> > 
> > Done.
> 
> No, it's not.  You removed the comment but didn't move the constant.

Done.

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.h#newcode212
base/mach_ipc_mac.h:212: protected:
On 2011/03/27 05:56:09, Peter Kasting wrote:
> Nit: While here, kill this extra "protected"

Done.

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.mm
File base/mach_ipc_mac.mm (right):

http://codereview.chromium.org/6688056/diff/15002/base/mach_ipc_mac.mm#newcode16
base/mach_ipc_mac.mm:16: sizeof(mach_msg_body_t) +
On 2011/03/27 05:56:09, Peter Kasting wrote:
> Nit: This indenting is strange.  Indent 4 and pull the third arg onto the same
> line, please.

Done.

[Peter Kasting, 2011-03-27 19:22:59]

LGTM with one nit

http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h#newcode181
base/mach_ipc_mac.h:181: static const size_t kEmptyMessageSize;
Nit: This actually goes at the very top, above the destructor (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
).  You don't need a blank line between it and the "public:" declaration.

[KushalP, 2011-03-27 20:18:27]

http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h
File base/mach_ipc_mac.h (right):

http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h#newcode181
base/mach_ipc_mac.h:181: static const size_t kEmptyMessageSize;
On 2011/03/27 19:22:59, Peter Kasting wrote:
> Nit: This actually goes at the very top, above the destructor (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> ).  You don't need a blank line between it and the "public:" declaration.

Done.

[KushalP, 2011-03-29 12:34:50]

Can this be closed/committed now?

On 2011/03/27 20:18:27, KushalP wrote:
> http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h
> File base/mach_ipc_mac.h (right):
> 
>
http://codereview.chromium.org/6688056/diff/20001/base/mach_ipc_mac.h#newcode181
> base/mach_ipc_mac.h:181: static const size_t kEmptyMessageSize;
> On 2011/03/27 19:22:59, Peter Kasting wrote:
> > Nit: This actually goes at the very top, above the destructor (see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> > ).  You don't need a blank line between it and the "public:" declaration.
> 
> Done.

[Peter Kasting, 2011-03-31 01:05:23]

On 2011/03/29 12:34:50, KushalP wrote:
> Can this be closed/committed now?

Yes, I just haven't had a chance to do it.  I will get to it tomorrow if another
committer does not get to it first.

[commit-bot: I haz the power, 2011-03-31 03:56:19]

Presubmit check for 6688056-21008 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  base/path_service.cc \
  base/mach_ipc_mac.mm \
  base/pickle.cc \
  base/shared_memory_posix.cc \
  base/mach_ipc_mac.h

Presubmit checks took 1.5s to calculate.

[Mohamed Mansour, 2011-03-31 04:00:31]

Kushal, if you don't mind, can you please modify the license header and place
"2011" for the years, then I can commit it cleanly :) Thanks.

[KushalP, 2011-03-31 11:24:01]

Sure thing! The license headers for the files that caused warnings have been
updated.

On 2011/03/31 04:00:31, Mohamed Mansour wrote:
> Kushal, if you don't mind, can you please modify the license header and place
> "2011" for the years, then I can commit it cleanly :) Thanks.

[Peter Kasting, 2011-03-31 20:34:31]

Landed in r80063, closing.