BIND 10 #846: STLPort regression in C++ libconfig

[BIND 10 Development]

#846: STLPort regression in C++ libconfig
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110503
                   Priority:         |            Resolution:
  critical                           |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  configuration                      |  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 first commit is unrelated fix, but I needed it
 to compile it with `-D_GLIBCXX_DEBUG` (so it provided me with a stacktrace
 in the time of the problem).

 Yeah, I've noticed we'd need it when I tried the stlport debug mode.
 Thanks for fixing it.

 > The fix itself should be simple ‒ provided that I guessed right the
 purpose of the original test function, which looked really strange. I
 don't think this needs a changelog entry, because the bug was in test only
 and the test didn't fail (mostly by coincidence, but the user should never
 have seen anything).

 Hmm, the original code was clearly wrong, and the proposed fix seems
 to correct it, and I've confirmed on the Solaris box with the debug
 mode of stlport.  But I wonder whether we really need the additional
 variable.  If the original intent is to remove the first element from
 the "list" that is identical to the given element, shouldn't it be
 sufficient to remove and return immediately we find it?

 {{{
     int i = 0;
     BOOST_FOREACH(ConstElementPtr s_el, list->listValue()) {
         if (*el == *s_el) {
             list->remove(i);
             return;
         }
         i++;
     }
 }}}

 Also (as mostly a matter of taste, and not really a comment on the
 patch itself), the mixture of counter variable and BOOST_FOREACH seem
 to be a bit awkward in that one motivation of this idiom in my
 understanding is to avoid having such an intermediate counter or
 iterator.  So, if we have to rely on help of additional state
 variable, why not just use iterators?

 {{{
     for (vector<ConstElementPtr>::const_iterator
 it(list->listValue().begin());
          it != list->listValue().end();
          ++it) {
         if (*el == **it) {
             list->remove(distance(list->listValue().begin(), it));
             return;
         }
     }
 }}}

 (of course it's a bit more inefficient due to the additional
 evaluation of end(), but in this context that wouldn't be a concern).

 And, in general, I'd prefer not using a loop to search for an item
 from a container because it inherently requests manipulating
 intermediate state variables, which are often a source of bug.  We
 could use STL algorithm instead:

 {{{
 struct ElementMatch {
     ElementMatch(ConstElementPtr key) : key_(key) {}
     bool operator()(ConstElementPtr element) const {
         return (*element == *key_);
     }
     ConstElementPtr key_;
 };

 void
 listRemove(ElementPtr list, ConstElementPtr el) {
     vector<ConstElementPtr>::const_iterator found =
         find_if(list->listValue().begin(), list->listValue().end(),
                 ElementMatch(el));
     if (found != list->listValue().end()) {
         list->remove(distance(list->listValue().begin(), found));
     }
 }
 }}}

 although, in this specific case, this is probably too much, and in
 fact not simpler and not so advantageous (it still needs to keep track
 of 'found' state).

 Anyway, these are quite minor.  If the argument for any of the
 alternative ways is reasonable and better than the current patch,
 please take it.  If you like your original patch most, I don't oppose
 that.

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