DelAppender - Argument Out Of Range Exception

[vveliu-csw]

Deleting appender at runtime, raises Argument Out Of Range Exception.

The problem is in the DelAppender procedure:

procedure TCustomLogWriter.DelAppender(const aAppender: ILogAppender);
.......
  for i := 0 to Self.FLoggerThread.FAppendersDecorators.Count - 1 do
    if Self.FLoggerThread.FAppendersDecorators[i].FLogAppender = aAppender then
      Self.FLoggerThread.FAppendersDecorators.Delete(i);
end;

The for statement loops till i = Self.FLoggerThread.FAppendersDecorators.Count - 1 , this is a mistake beacuse if we delete some Appender contained in the list the higher limit of the loop is not re assigned. In a list of 3 items for will loop from 0 to 2 but if we delete an item the loop don't stop so trying to read/write List[2] will raise the exception.

I used a repeat .. until .. statement instead, the loop should stop after succesfull delete (appender is supposed to exists only once in the list).

Modified version of mine:

procedure TCustomLogWriter.DelAppender(const aAppender: ILogAppender);
var
  i: Integer;
  LAppenderDeleted : Boolean;
begin
  i := Self.FLogAppenders.IndexOf(aAppender);
  if i >= 0 then
  begin
    Self.FLogAppenders.Delete(i);
  end;

  if Self.FLoggerThread.FAppendersDecorators.Count = 0 then
  begin
    Exit;
  end;

  i := 0;
  LAppenderDeleted := False;

  repeat
    if Self.FLoggerThread.FAppendersDecorators[i].FLogAppender = aAppender then
    begin
      Self.FLoggerThread.FAppendersDecorators.Delete(i);
      LAppenderDeleted := True;
    end;

    Inc(i);
  until (LAppenderDeleted or (i = Self.FLoggerThread.FAppendersDecorators.Count));
end;

Plus. I removed the above code beacuse Self.FLogAppenders and Self.FLoggerThread.FAppenders are the same list:

  i := Self.FLoggerThread.FAppenders.IndexOf(aAppender);
  if i >= 0 then
    Self.FLoggerThread.FAppenders.Delete(i);

[luebbe]

For deleting items from lists, I always count down from the last element to zero so the index is always valid. Saves you the boolean for the repeat termination.

Like (untested from the top of my head) this:

for i := FLoggerThread.Appenders.Count -1 downto 0 do 
  if Self.FLoggerThread.FAppendersDecorators[i].FLogAppender = aAppender then
    begin
      Self.FLoggerThread.FAppendersDecorators.Delete(i);
      break;
    end;

[vveliu-csw]

@luebbe Edit: I get it! Your version of the loop allways evaluates the Count property so in case of delete the decrease of count prevent the index to go out of range.

It's true that you can break the loop after the delete but I think for is intended to loop trought the entire range. repeat until on the other hand can stop at some point. This is my personal opinion!