Skip to content

Commit

Permalink
Remove comparisons of this with NULL.
Browse files Browse the repository at this point in the history
Calling functions on objects that are NULL is undefined behavior.
Hence, new versions of clang assume that `this` is never NULL,
warn on comparisons of `this` with NULL, and optimize the comparison away.

There were three comparisons of `this` with NULL in re2:

1. CharClass::Delete(). There are only two callers of this method, and
   as far as I can tell `this` can't be NULL there, so just delete that check.
2. Prefilter::DebugString(). This too has two callers: Prefilter::Info::ToString(),
   which already checks for NULL before calling, and DebugString() itself.
   Since several places check Prefilters for NULL, add explicit checks before
   calling here.
3. Prefilter::Info::ToString(). This has three callers. In 2 cases this can't be
   NULL. In the third case it could conceivably be NULL if op() is
   kRegexpConcat and nchild_args is 0, so add an explicit check here.

Fixes https://code.google.com/p/re2/issues/detail?id=115

LGTM=rsc
R=rsc
CC=re2-dev
https://codereview.appspot.com/107100043
  • Loading branch information
nico authored and rsc committed Jun 26, 2014
1 parent 7bd3b3b commit 1aef6f5
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 16 deletions.
1 change: 1 addition & 0 deletions CONTRIBUTORS
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Doug Kwan <dougkwan@google.com>
Dmitriy Vyukov <dvyukov@google.com>
John Millikin <jmillikin@gmail.com>
Mike Nazarewicz <mpn@google.com>
Nico Weber <thakis@chromium.org>
Pawel Hajdan <phajdan.jr@gmail.com>
Rob Pike <r@google.com>
Russ Cox <rsc@swtch.com>
Expand Down
19 changes: 5 additions & 14 deletions re2/prefilter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -265,14 +265,6 @@ Prefilter* Prefilter::Info::TakeMatch() {

// Format a Info in string form.
string Prefilter::Info::ToString() {
if (this == NULL) {
// Sometimes when iterating on children of a node,
// some children might have NULL Info. Adding
// the check here for NULL to take care of cases where
// the caller is not checking.
return "";
}

if (is_exact_) {
int n = 0;
string s;
Expand Down Expand Up @@ -640,7 +632,7 @@ Prefilter::Info* Prefilter::Info::Walker::PostVisit(

if (Trace) {
VLOG(0) << "BuildInfo " << re->ToString()
<< ": " << info->ToString();
<< ": " << (info ? info->ToString() : "");
}

return info;
Expand All @@ -665,9 +657,6 @@ Prefilter* Prefilter::FromRegexp(Regexp* re) {
}

string Prefilter::DebugString() const {
if (this == NULL)
return "<nil>";

switch (op_) {
default:
LOG(DFATAL) << "Bad op in Prefilter::DebugString: " << op_;
Expand All @@ -683,7 +672,8 @@ string Prefilter::DebugString() const {
for (int i = 0; i < subs_->size(); i++) {
if (i > 0)
s += " ";
s += (*subs_)[i]->DebugString();
Prefilter* sub = (*subs_)[i];
s += sub ? sub->DebugString() : "<nil>";
}
return s;
}
Expand All @@ -692,7 +682,8 @@ string Prefilter::DebugString() const {
for (int i = 0; i < subs_->size(); i++) {
if (i > 0)
s += "|";
s += (*subs_)[i]->DebugString();
Prefilter* sub = (*subs_)[i];
s += sub ? sub->DebugString() : "<nil>";
}
s += ")";
return s;
Expand Down
2 changes: 0 additions & 2 deletions re2/regexp.cc
Original file line number Diff line number Diff line change
Expand Up @@ -873,8 +873,6 @@ CharClass* CharClass::New(int maxranges) {
}

void CharClass::Delete() {
if (this == NULL)
return;
uint8 *data = reinterpret_cast<uint8*>(this);
delete[] data;
}
Expand Down

0 comments on commit 1aef6f5

Please sign in to comment.