More DCHECK() updates. A mixture of _EQ and _GE.

More DCHECK() updates. A mixture of _EQ and _GE.

Bug=58409
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76343

[Mohamed Mansour, 2011-02-19 20:38:08]

LGTM, I will let William see if he likes this approach. I believe he does.

[KushalP, 2011-02-21 18:32:08]

Can this be tested on the tryservers then? (please)

On 2011/02/19 20:38:08, Mohamed Mansour wrote:
> LGTM, I will let William see if he likes this approach. I believe he does.

[Mohamed Mansour, 2011-02-21 19:13:17]

Okay, already did :)

Results will be emailed to: kushi.p@gmail.com
Patch 'master#27b9fa' sent to try server: win, mac, linux


Kind regards,
Mohamed Mansour



On Mon, Feb 21, 2011 at 1:32 PM, <kushi.p@gmail.com> wrote:

> Reviewers: Mohamed Mansour, willchan,
>
> Message:
> Can this be tested on the tryservers then? (please)
>
>
> On 2011/02/19 20:38:08, Mohamed Mansour wrote:
>
>> LGTM, I will let William see if he likes this approach. I believe he does.
>>
>
>
>
> Description:
> More DCHECK() updates. A mixture of _EQ and _GE.
>
> Bug=58409
>
> Please review this at http://codereview.chromium.org/6469070/
>
> SVN Base: http://git.chromium.org/git/chromium.git@trunk
>
> Affected files:
>  M base/event_recorder.cc
>  M base/i18n/bidi_line_iterator.cc
>  M base/message_pump_libevent.cc
>  M base/message_pump_win.cc
>  M base/pickle.h
>  M base/platform_file_posix.cc
>  M base/shared_memory_posix.cc
>  M base/shared_memory_win.cc
>  M base/synchronization/condition_variable_posix.cc
>  M base/synchronization/waitable_event_win.cc
>  M base/threading/thread_local_storage_win.cc
>  M base/tracked_objects.cc
>  M base/win/scoped_bstr.cc
>  M base/win/scoped_comptr.h
>  M base/win/scoped_variant.cc
>
>
> Index: base/event_recorder.cc
> diff --git a/base/event_recorder.cc b/base/event_recorder.cc
> index
>
7c3336bde83d1020c2f39ae53e49ce79e81963d4..1119b4f57c0e676bff7ae3a34ee7f3b71f3461e6
> 100644
> --- a/base/event_recorder.cc
> +++ b/base/event_recorder.cc
> @@ -49,7 +49,7 @@ bool EventRecorder::StartRecording(const FilePath&
> filename) {
>     return false;
>
>   // Open the recording file.
> -  DCHECK(file_ == NULL);
> +  DCHECK(!file_);
>   file_ = file_util::OpenFile(filename, "wb+");
>   if (!file_) {
>     DLOG(ERROR) << "EventRecorder could not open log file";
> @@ -100,7 +100,7 @@ bool EventRecorder::StartPlayback(const FilePath&
> filename) {
>     return false;
>
>   // Open the recording file.
> -  DCHECK(file_ == NULL);
> +  DCHECK(!file_);
>   file_ = file_util::OpenFile(filename, "rb");
>   if (!file_) {
>     DLOG(ERROR) << "EventRecorder Playback could not open log file";
> Index: base/i18n/bidi_line_iterator.cc
> diff --git a/base/i18n/bidi_line_iterator.cc
> b/base/i18n/bidi_line_iterator.cc
> index
>
3222a3ad136dadf0bd08d70aa389ff72974dce93..5e9cd11c23eb96271be485369d840a49c8dc3c5c
> 100644
> --- a/base/i18n/bidi_line_iterator.cc
> +++ b/base/i18n/bidi_line_iterator.cc
> @@ -22,7 +22,7 @@ BiDiLineIterator::~BiDiLineIterator() {
>  bool BiDiLineIterator::Open(const string16& text,
>                             bool right_to_left,
>                             bool url) {
> -  DCHECK(bidi_ == NULL);
> +  DCHECK(!bidi_);
>   UErrorCode error = U_ZERO_ERROR;
>   bidi_ = ubidi_openSized(static_cast<int>(text.length()), 0, &error);
>   if (U_FAILURE(error))
> Index: base/message_pump_libevent.cc
> diff --git a/base/message_pump_libevent.cc b/base/message_pump_libevent.cc
> index
>
933d7950eacd331fb08bdb70e4dc3c54187a9206..28b4bfe91352434d9884ad9277a8811651ab9d9f
> 100644
> --- a/base/message_pump_libevent.cc
> +++ b/base/message_pump_libevent.cc
> @@ -78,7 +78,7 @@ bool
> MessagePumpLibevent::FileDescriptorWatcher::StopWatchingFileDescriptor() {
>  void MessagePumpLibevent::FileDescriptorWatcher::Init(event *e,
>                                                       bool is_persistent) {
>   DCHECK(e);
> -  DCHECK(event_ == NULL);
> +  DCHECK(!event_);
>
>   is_persistent_ = is_persistent;
>   event_ = e;
> Index: base/message_pump_win.cc
> diff --git a/base/message_pump_win.cc b/base/message_pump_win.cc
> index
>
d3f36218cac5779299bb970a70768bc6b934f8c3..73fc3aee2c1cf8d0e0a85a6e00f44c5e4e550026
> 100644
> --- a/base/message_pump_win.cc
> +++ b/base/message_pump_win.cc
> @@ -470,7 +470,7 @@ void MessagePumpForIO::DoRunLoop() {
>  void MessagePumpForIO::WaitForWork() {
>   // We do not support nested IO message loops. This is to avoid messy
>   // recursion problems.
> -  DCHECK(state_->run_depth == 1) << "Cannot nest an IO message loop!";
> +  DCHECK_EQ(state_->run_depth, 1) << "Cannot nest an IO message loop!";
>
>   int timeout = GetCurrentDelay();
>   if (timeout < 0)  // Negative value means no timers waiting.
> Index: base/pickle.h
> diff --git a/base/pickle.h b/base/pickle.h
> index
>
ad08b7c0af0156131120bc0c875412705402d31d..305bef47648300c8addd207b6326d26aac717184
> 100644
> --- a/base/pickle.h
> +++ b/base/pickle.h
> @@ -143,12 +143,12 @@ class Pickle {
>   // to the Pickle constructor.
>   template <class T>
>   T* headerT() {
> -    DCHECK(sizeof(T) == header_size_);
> +    DCHECK_EQ(sizeof(T), header_size_);
>     return static_cast<T*>(header_);
>   }
>   template <class T>
>   const T* headerT() const {
> -    DCHECK(sizeof(T) == header_size_);
> +    DCHECK_EQ(sizeof(T), header_size_);
>     return static_cast<const T*>(header_);
>   }
>
> Index: base/platform_file_posix.cc
> diff --git a/base/platform_file_posix.cc b/base/platform_file_posix.cc
> index
>
38989c6bf41cddcf5b961b31cbcbfc6782df5d9d..266fa5a4b0646515bc03b032df6ee6f26d5edb35
> 100644
> --- a/base/platform_file_posix.cc
> +++ b/base/platform_file_posix.cc
> @@ -65,7 +65,7 @@ PlatformFile CreatePlatformFile(const FilePath& name, int
> flags,
>     open_flags |= O_TRUNC;
>   }
>
> -  DCHECK(O_RDONLY == 0);
> +  DCHECK_EQ(O_RDONLY, 0U);
>
>   int descriptor = open(name.value().c_str(), open_flags, S_IRUSR |
> S_IWUSR);
>
> Index: base/shared_memory_posix.cc
> diff --git a/base/shared_memory_posix.cc b/base/shared_memory_posix.cc
> index
>
843322b69801817458577ed7eaa92871497ff0d9..bf9442aba22565a6255d38a35374f393ffe0fef6
> 100644
> --- a/base/shared_memory_posix.cc
> +++ b/base/shared_memory_posix.cc
> @@ -98,7 +98,7 @@ bool SharedMemory::CreateAnonymous(uint32 size) {
>  // of mem_filename after FilePathForMemoryName().
>  bool SharedMemory::CreateNamed(const std::string& name,
>                                bool open_existing, uint32 size) {
> -  DCHECK(mapped_file_ == -1);
> +  DCHECK_EQ(mapped_file_, -1);
>   if (size == 0) return false;
>
>   // This function theoretically can block on the disk, but realistically
> @@ -243,7 +243,7 @@ void SharedMemory::Unlock() {
>  }
>
>  bool SharedMemory::PrepareMapFile(FILE *fp) {
> -  DCHECK(mapped_file_ == -1);
> +  DCHECK_EQ(mapped_file_, -1);
>   if (fp == NULL) return false;
>
>   // This function theoretically can block on the disk, but realistically
> @@ -278,8 +278,8 @@ bool SharedMemory::FilePathForMemoryName(const
> std::string& mem_name,
>                                          FilePath* path) {
>   // mem_name will be used for a filename; make sure it doesn't
>   // contain anything which will confuse us.
> -  DCHECK(mem_name.find('/') == std::string::npos);
> -  DCHECK(mem_name.find('\0') == std::string::npos);
> +  DCHECK_EQ(mem_name.find('/'), std::string::npos);
> +  DCHECK_EQ(mem_name.find('\0'), std::string::npos);
>
>   FilePath temp_dir;
>   if (!file_util::GetShmemTempDir(&temp_dir))
> Index: base/shared_memory_win.cc
> diff --git a/base/shared_memory_win.cc b/base/shared_memory_win.cc
> index
>
15c61ddf00def5295ffb857cd1082d82c27c4551..526132c28517f90591c3d740372dd44b5e50166d
> 100644
> --- a/base/shared_memory_win.cc
> +++ b/base/shared_memory_win.cc
> @@ -80,7 +80,7 @@ bool SharedMemory::CreateAnonymous(uint32 size) {
>
>  bool SharedMemory::CreateNamed(const std::string& name,
>                                bool open_existing, uint32 size) {
> -  DCHECK(mapped_file_ == NULL);
> +  DCHECK(!mapped_file_);
>   if (size == 0)
>     return false;
>
> @@ -119,7 +119,7 @@ bool SharedMemory::Delete(const std::string& name) {
>  }
>
>  bool SharedMemory::Open(const std::string& name, bool read_only) {
> -  DCHECK(mapped_file_ == NULL);
> +  DCHECK(!mapped_file_);
>
>   name_ = ASCIIToWide(name);
>   read_only_ = read_only;
> Index: base/synchronization/condition_variable_posix.cc
> diff --git a/base/synchronization/condition_variable_posix.cc
> b/base/synchronization/condition_variable_posix.cc
> index
>
eff7053c893e0ad79911dd68fef1b893971afedc..c02bf9f0dae035e75ff8fa602ce025546c5788a6
> 100644
> --- a/base/synchronization/condition_variable_posix.cc
> +++ b/base/synchronization/condition_variable_posix.cc
> @@ -20,12 +20,12 @@ ConditionVariable::ConditionVariable(Lock* user_lock)
>  #endif
>  {
>   int rv = pthread_cond_init(&condition_, NULL);
> -  DCHECK(rv == 0);
> +  DCHECK_EQ(rv, 0);
>  }
>
>  ConditionVariable::~ConditionVariable() {
>   int rv = pthread_cond_destroy(&condition_);
> -  DCHECK(rv == 0);
> +  DCHECK_EQ(rv, 0);
>  }
>
>  void ConditionVariable::Wait() {
> @@ -33,7 +33,7 @@ void ConditionVariable::Wait() {
>   user_lock_->CheckHeldAndUnmark();
>  #endif
>   int rv = pthread_cond_wait(&condition_, user_mutex_);
> -  DCHECK(rv == 0);
> +  DCHECK_EQ(rv, 0);
>  #if !defined(NDEBUG)
>   user_lock_->CheckUnheldAndMark();
>  #endif
> @@ -52,7 +52,7 @@ void ConditionVariable::TimedWait(const TimeDelta&
> max_time) {
>                     Time::kNanosecondsPerMicrosecond;
>   abstime.tv_sec += abstime.tv_nsec / Time::kNanosecondsPerSecond;
>   abstime.tv_nsec %= Time::kNanosecondsPerSecond;
> -  DCHECK(abstime.tv_sec >= now.tv_sec);  // Overflow paranoia
> +  DCHECK_GE(abstime.tv_sec, now.tv_sec);  // Overflow paranoia
>
>  #if !defined(NDEBUG)
>   user_lock_->CheckHeldAndUnmark();
> @@ -66,12 +66,12 @@ void ConditionVariable::TimedWait(const TimeDelta&
> max_time) {
>
>  void ConditionVariable::Broadcast() {
>   int rv = pthread_cond_broadcast(&condition_);
> -  DCHECK(rv == 0);
> +  DCHECK_EQ(rv, 0);
>  }
>
>  void ConditionVariable::Signal() {
>   int rv = pthread_cond_signal(&condition_);
> -  DCHECK(rv == 0);
> +  DCHECK_EQ(rv, 0);
>  }
>
>  }  // namespace base
> Index: base/synchronization/waitable_event_win.cc
> diff --git a/base/synchronization/waitable_event_win.cc
> b/base/synchronization/waitable_event_win.cc
> index
>
0fcf488eb1e9b918cd11d0315c68b5317db6233b..1321d9828bcfa6ec2578d44318198a0ccec1fced
> 100644
> --- a/base/synchronization/waitable_event_win.cc
> +++ b/base/synchronization/waitable_event_win.cc
> @@ -50,7 +50,7 @@ bool WaitableEvent::Wait() {
>   DWORD result = WaitForSingleObject(handle_, INFINITE);
>   // It is most unexpected that this should ever fail.  Help consumers
> learn
>   // about it if it should ever fail.
> -  DCHECK(result == WAIT_OBJECT_0) << "WaitForSingleObject failed";
> +  DCHECK_EQ(result, WAIT_OBJECT_0) << "WaitForSingleObject failed";
>   return result == WAIT_OBJECT_0;
>  }
>
> Index: base/threading/thread_local_storage_win.cc
> diff --git a/base/threading/thread_local_storage_win.cc
> b/base/threading/thread_local_storage_win.cc
> index
>
667d1b93568146d92a7c1a12b30c8e9643c37cc7..c8128f1c06df4cd894c184d0368e612b4ebfdb47
> 100644
> --- a/base/threading/thread_local_storage_win.cc
> +++ b/base/threading/thread_local_storage_win.cc
> @@ -47,7 +47,7 @@ void** ThreadLocalStorage::Initialize() {
>       TlsFree(value);
>     }
>   }
> -  DCHECK(TlsGetValue(tls_key_) == NULL);
> +  DCHECK(!TlsGetValue(tls_key_));
>
>   // Create an array to store our data.
>   void** tls_data = new void*[kThreadLocalStorageSize];
> Index: base/tracked_objects.cc
> diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc
> index
>
a9f81b1a71ac65ad4282d74feabbec45a4a229e3..ef5b918278e165e29f2a616ba884f9bf2f006348
> 100644
> --- a/base/tracked_objects.cc
> +++ b/base/tracked_objects.cc
> @@ -467,7 +467,7 @@ bool ThreadData::StartTracking(bool status) {
>     return true;
>   }
>   base::AutoLock lock(list_lock_);
> -  DCHECK(status_ == UNINITIALIZED);
> +  DCHECK_EQ(status_, UNINITIALIZED);
>   CHECK(tls_index_.Initialize(NULL));
>   status_ = ACTIVE;
>   return true;
> Index: base/win/scoped_bstr.cc
> diff --git a/base/win/scoped_bstr.cc b/base/win/scoped_bstr.cc
> index
>
18ffe6abe0d591d6eba97334db458751868e2346..63ade0cb4215ae6deeffb6ca6937159ba180d76c
> 100644
> --- a/base/win/scoped_bstr.cc
> +++ b/base/win/scoped_bstr.cc
> @@ -39,7 +39,7 @@ void ScopedBstr::Swap(ScopedBstr& bstr2) {
>  }
>
>  BSTR* ScopedBstr::Receive() {
> -  DCHECK(bstr_ == NULL) << "BSTR leak.";
> +  DCHECK(!bstr_) << "BSTR leak.";
>   return &bstr_;
>  }
>
> Index: base/win/scoped_comptr.h
> diff --git a/base/win/scoped_comptr.h b/base/win/scoped_comptr.h
> index
>
6375218822df5436cb64ef3fc540ca3880e71750..e508ef2ce7ce31c561527d911f1e2032b292dced
> 100644
> --- a/base/win/scoped_comptr.h
> +++ b/base/win/scoped_comptr.h
> @@ -69,7 +69,7 @@ class ScopedComPtr : public scoped_refptr<Interface> {
>
>   // Accepts an interface pointer that has already been addref-ed.
>   void Attach(Interface* p) {
> -    DCHECK(ptr_ == NULL);
> +    DCHECK(!ptr_);
>     ptr_ = p;
>   }
>
> @@ -78,7 +78,7 @@ class ScopedComPtr : public scoped_refptr<Interface> {
>   // The function DCHECKs on the current value being NULL.
>   // Usage: Foo(p.Receive());
>   Interface** Receive() {
> -    DCHECK(ptr_ == NULL) << "Object leak. Pointer must be NULL";
> +    DCHECK(!ptr_) << "Object leak. Pointer must be NULL";
>     return &ptr_;
>   }
>
> @@ -114,7 +114,7 @@ class ScopedComPtr : public scoped_refptr<Interface> {
>   // Convenience wrapper around CoCreateInstance
>   HRESULT CreateInstance(const CLSID& clsid, IUnknown* outer = NULL,
>                          DWORD context = CLSCTX_ALL) {
> -    DCHECK(ptr_ == NULL);
> +    DCHECK(!ptr_);
>     HRESULT hr = ::CoCreateInstance(clsid, outer, context, *interface_id,
>                                     reinterpret_cast<void**>(&ptr_));
>     return hr;
> Index: base/win/scoped_variant.cc
> diff --git a/base/win/scoped_variant.cc b/base/win/scoped_variant.cc
> index
>
40ccdf22d87f9287d215dd6c4d4ee9390bc9cb58..f57ab93f6c66761b006b305223cad0b9139361d4
> 100644
> --- a/base/win/scoped_variant.cc
> +++ b/base/win/scoped_variant.cc
> @@ -211,7 +211,7 @@ void ScopedVariant::Set(SAFEARRAY* array) {
>     var_.vt |= VT_ARRAY;
>     var_.parray = array;
>   } else {
> -    DCHECK(array == NULL) << "Unable to determine safearray vartype";
> +    DCHECK(!array) << "Unable to determine safearray vartype";
>     var_.vt = VT_EMPTY;
>   }
>  }
>
>
>

[willchan no longer on Chromium, 2011-02-21 23:17:20]

LGTM

http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:68: DCHECK_EQ(O_RDONLY, 0U);
Strange. O_RDONLY is a constant. Why do we have a runtime assertion to make sure
two constants are equal?

Perhaps we should use a COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY_must_equal_zero);
compile-time assertion instead.

[KushalP, 2011-02-22 00:57:28]

http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:68: DCHECK_EQ(O_RDONLY, 0U);
Sure, I'll look into it.

Isn't the second argument supposed to be a string message to be thrown and not
another variable?

On 2011/02/21 23:17:20, willchan wrote:
> Strange. O_RDONLY is a constant. Why do we have a runtime assertion to make
sure
> two constants are equal?
> 
> Perhaps we should use a COMPILE_ASSERT(O_RDONLY == 0,
O_RDONLY_must_equal_zero);
> compile-time assertion instead.

[willchan no longer on Chromium, 2011-02-22 01:45:22]

On Feb 21, 2011 4:57 PM, <kushi.p@gmail.com> wrote:
>
>
> http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc
> File base/platform_file_posix.cc (right):
>
>
http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc#new...
> base/platform_file_posix.cc:68: DCHECK_EQ(O_RDONLY, 0U);
> Sure, I'll look into it.
>
> Isn't the second argument supposed to be a string message to be thrown
> and not another variable?
>

The macro works by declaring an invalid variable. So the variable name is
the error string.

>
> On 2011/02/21 23:17:20, willchan wrote:
>>
>> Strange. O_RDONLY is a constant. Why do we have a runtime assertion to
>
> make sure
>>
>> two constants are equal?
>
>
>> Perhaps we should use a COMPILE_ASSERT(O_RDONLY == 0,
>
> O_RDONLY_must_equal_zero);
>>
>> compile-time assertion instead.
>
>
> http://codereview.chromium.org/6469070/

[KushalP, 2011-02-22 13:13:18]

Have updated, with a new patch set

On 2011/02/22 01:45:22, willchan wrote:
> On Feb 21, 2011 4:57 PM, <mailto:kushi.p@gmail.com> wrote:
> >
> >
> > http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc
> > File base/platform_file_posix.cc (right):
> >
> >
>
http://codereview.chromium.org/6469070/diff/1/base/platform_file_posix.cc#new...
> > base/platform_file_posix.cc:68: DCHECK_EQ(O_RDONLY, 0U);
> > Sure, I'll look into it.
> >
> > Isn't the second argument supposed to be a string message to be thrown
> > and not another variable?
> >
> 
> The macro works by declaring an invalid variable. So the variable name is
> the error string.
> 
> >
> > On 2011/02/21 23:17:20, willchan wrote:
> >>
> >> Strange. O_RDONLY is a constant. Why do we have a runtime assertion to
> >
> > make sure
> >>
> >> two constants are equal?
> >
> >
> >> Perhaps we should use a COMPILE_ASSERT(O_RDONLY == 0,
> >
> > O_RDONLY_must_equal_zero);
> >>
> >> compile-time assertion instead.
> >
> >
> > http://codereview.chromium.org/6469070/

[willchan no longer on Chromium, 2011-02-22 19:54:59]

LGTM

[KushalP, 2011-02-27 23:57:49]

I've updated the patchset to follow the EXPECTED, ACTUAL pattern as pkasting
stated in the Issue thread. This should be good to close/commit now.

On 2011/02/22 19:54:59, willchan wrote:
> LGTM

[willchan no longer on Chromium, 2011-02-28 21:35:22]

mhm: can you land this?

On Sun, Feb 27, 2011 at 3:57 PM,  <kushi.p@gmail.com> wrote:
> I've updated the patchset to follow the EXPECTED, ACTUAL pattern as pkasting
> stated in the Issue thread. This should be good to close/commit now.
>
> On 2011/02/22 19:54:59, willchan wrote:
>>
>> LGTM
>
>
>
> http://codereview.chromium.org/6469070/
>

[Peter Kasting, 2011-02-28 21:42:13]

Kinda makes me sad that all the "== NULL" checks are being replaced by "!",
which seems slightly less clear to me, but we don't have a style rule about it,
so whatever,

http://codereview.chromium.org/6469070/diff/8001/base/pickle.h
File base/pickle.h (right):

http://codereview.chromium.org/6469070/diff/8001/base/pickle.h#newcode146
base/pickle.h:146: DCHECK_EQ(sizeof(T), header_size_);
Nit: I think the order here should be reversed

[KushalP, 2011-02-28 21:54:06]

> Kinda makes me sad that all the "== NULL" checks are being replaced by "!",

I think I asked around on IRC about what was preferred and ! over == NULL was
given. If it's that big a deal, I can revert, however it'd be great to have some
clarification before I continue. Especially with the various DCHECK_ use cases.

http://codereview.chromium.org/6469070/diff/8001/base/pickle.h
File base/pickle.h (right):

http://codereview.chromium.org/6469070/diff/8001/base/pickle.h#newcode146
base/pickle.h:146: DCHECK_EQ(sizeof(T), header_size_);
On 2011/02/28 21:42:13, Peter Kasting wrote:
> Nit: I think the order here should be reversed

Done.

[Peter Kasting, 2011-02-28 21:55:37]

On 2011/02/28 21:54:06, KushalP wrote:
> > Kinda makes me sad that all the "== NULL" checks are being replaced by "!",
> 
> I think I asked around on IRC about what was preferred and ! over == NULL was
> given. If it's that big a deal, I can revert, however it'd be great to have
some
> clarification before I continue. Especially with the various DCHECK_ use
cases.

I would say that both are OK and therefore I'm not a fan of changing either one
to the other.

However I can understand why changing them can make your life easier w.r.t.
finding what places to update.

[willchan no longer on Chromium, 2011-02-28 21:56:43]

On Mon, Feb 28, 2011 at 1:54 PM,  <kushi.p@gmail.com> wrote:
>> Kinda makes me sad that all the "== NULL" checks are being replaced by
>> "!",
>
> I think I asked around on IRC about what was preferred and ! over == NULL
> was
> given. If it's that big a deal, I can revert, however it'd be great to have
> some
> clarification before I continue. Especially with the various DCHECK_ use
> cases.

I didn't comment on these because it's more ambiguous and you're often
up to the whim to the reviewer on these. I personally try to minimize
commenting on these types of nits since some people prefer one style
and others another. I would also say that you shouldn't feel like you
need to change these, as there isn't a noticeable win either way.

>
>
> http://codereview.chromium.org/6469070/diff/8001/base/pickle.h
> File base/pickle.h (right):
>
> http://codereview.chromium.org/6469070/diff/8001/base/pickle.h#newcode146
> base/pickle.h:146: DCHECK_EQ(sizeof(T), header_size_);
> On 2011/02/28 21:42:13, Peter Kasting wrote:
>>
>> Nit: I think the order here should be reversed
>
> Done.
>
> http://codereview.chromium.org/6469070/
>

[Mohamed Mansour, 2011-02-28 22:59:30]

On 2011/02/28 21:35:22, willchan wrote:
> mhm: can you land this?
> 

willchan: yes I can land this, do we still want it, or we would rather have the
"== NULL" and make it a style rule for Chrome. I am free right now, so lgtm it
if you want this style.

[willchan no longer on Chromium, 2011-02-28 23:01:06]

LGTM as is. I don't think the == NULL vs ! is important enough to add
to the style guide. I'd LGTM'd either way.

On Mon, Feb 28, 2011 at 2:59 PM,  <mhm@chromium.org> wrote:
> On 2011/02/28 21:35:22, willchan wrote:
>>
>> mhm: can you land this?
>
>
> willchan: yes I can land this, do we still want it, or we would rather have
> the
> "== NULL" and make it a style rule for Chrome. I am free right now, so lgtm
> it
> if you want this style.
>
>
> http://codereview.chromium.org/6469070/
>