BIND 10 #847: STLPort regression in b10-resolver

[BIND 10 Development]

#847: STLPort regression in b10-resolver
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110503
                   Priority:         |            Resolution:
  critical                           |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  resolver                           |  Estimated Difficulty:  0.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:3 vorner]:

 > It should be fixed. The problem was, when the iterator got invalidated
 by the remove, a variable was set so another iteration of the loop was not
 entered, but the iterator was increased once before checking the
 condition. It should be harmless in real life, since the vector iterator
 is only a pointer, so increasing it and not using doesn't matter, but I
 fixed it when I found it.

 The patch seems to correctly fix the reported problem, and I confirmed
 that on the Solaris box.  So please feel free to merge it.

 A supplemental comment not directly related to the patch: I'd reverse
 the semantics and name of variable 'nomatch', i.e., name it 'match'
 with the default value of false, set it to 'true' when the if
 condition in the inner most for loop is met, and remove the entry if
 !match.  I'd leave it to you whether to make this change when you
 merge it.

 I have even a higher level comment (again, not specific to this
 patch).  IMO this method is too complicated and tricky to understand.
 It's difficult to follow how the triple-nested loops work, with
 trucking subtle implication of various non constant variables, kept,
 (no)match, removed.  If we make the RRsetIterators more complete, I
 believe we can simplify the whole logic and the resulting code will be
 more understandably.  For example, if we add something like
 vector::erase() to Message (or revised Message::removeRRset() that
 way), we'd be able to update the entire code like this:

 {{{
     message.eraseSection(remove_if(message.beginSection(section),
                                    message.endSection(section),
                                    NameMatch(names)),
                          message.endSection(section));
 }}}

 (where NameMatch class has operator() that implements the match logic
 in the innermost for loop of the original code).

 I understand this is far beyond the scope of this ticket, so I'd
 propose creating a separate ticket for this.

-- 
Ticket URL: <http://bind10.isc.org/ticket/847#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development