DomainIntVar::RemoveValue

[GoogleCodeExporter]

I was going through the code of the DomainIntVar::RemoveValue method and there 
are two things I find suspicious. The function goes like this (taken from 
trunk) :

---------------------------

void DomainIntVar::RemoveValue(int64 v)  {
  if (v < min_ || v > max_)
    return;
  if (v == min_) {
    SetMin(v + 1);
  } else if (v == max_) {
    SetMax(v - 1);
  } else {
    if (bits_ == NULL) {
      CreateBits();
    }
    if (in_process_ && v >= new_min_ && v <= new_max_ && bits_->Contains(v)) {
      bits_->DelayRemoveValue(v);
    } else {
      if (bits_->RemoveValue(v)) {
        Push();
      }
    }
  }
}

---------------------------

1) If the variable is "in process", shouldn't you test v with new_min_ and 
new_max_ instead of min_ and max_?

2) In the last if statement, shouldn't it be more like this?

if (in_process_) {
  if (v >= new_min_ && v <= new_max_ && bits_->Contains(v)) {
      bits_->DelayRemoveValue(v);
  }
} else {
  if (bits_->RemoveValue(v)) {
    Push();
  }
}

So to summarize I think the patch would be :

void DomainIntVar::RemoveValue(int64 v)  {

  if (in_process_) {

    if (v < new_min_ || v > new_max_) {
      return;
    } else if (v == new_min_ && v == new_max_) {
      solver()->Fail();
    } else if (v == new_min_ ) {
      new_min_++; 
    } else if (v == new_max_) {
      new_max_--;
    } else {
      if (!bits_) CreateBits();
      bits_->DelayRemoveValue(v);
    }

  } else {

    if (v < min_ || v > max_) {
      return;
    } else if (v == min_) {
      SetMin(v + 1);
    } else if (v == max_) {
      SetMax(v - 1);
    } else {
      if (bits_ == NULL) CreateBits();
      if (bits_->RemoveValue(v)) Push();
    }

  }
}

Maybe I'm wrong but I just wanted to let you know in case it's really a problem!

Philippe Van Kessel

Original issue reported on code.google.com by phil.v...@gmail.com on 28 Nov 2011 at 8:52

[GoogleCodeExporter]

Hello, 

I have implemented the simple fix (nested if).
The big split does not bring a lot IMO.

Thanks!!!

Original comment by laurent....@gmail.com on 9 Dec 2011 at 6:35

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by laurent....@gmail.com on 4 Apr 2014 at 12:18

• Changed state: Fixed