Opened 9 years ago

Closed 9 years ago

#846 closed defect (fixed)

STLPort regression in C++ libconfig

Reported by: jinmei Owned by: vorner
Priority: high Milestone: Sprint-20110503
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The debug mode of STLPort reported the following bugs in src/lib/config/tests:

[ RUN      ] CCSessionTest.checkCommand
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(25): STL error : Past-the-end iterator could not be erased
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(25): STL assertion failure:     __position._M_iterator !=this->_M_finish 

[ RUN      ] CCSessionTest.checkCommand2
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(250): STL error : Past-the-end iterator could not be erased
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(250): STL assertion failure:     __position._M_iterator !=this->_M_finish

[ RUN      ] CCSessionTest.ignoreRemoteConfigCommands
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(250): STL error : Past-the-end iterator could not be erased
/export/home/opt/sunstudio12.1/prod/include/CC/stlport4/stl/debug/_vector.h(250): STL assertion failure:     __position._M_iterator !=this->_M_finish

Subtickets

Change History (12)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from Year 3 Task Backlog to Sprint-20110503

comment:2 Changed 9 years ago by vorner

  • Defect Severity set to N/A
  • Owner set to vorner
  • Status changed from new to accepted
  • Sub-Project set to DNS

comment:3 follow-ups: Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

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). The debug mode discovered bunch of unrelated errors as well, so I'm going to have a look trough trac and see if they are already reported or not.

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).

comment:4 in reply to: ↑ 3 Changed 9 years ago by jinmei

Replying to vorner:

Hello

It should be fixed.

Have you pushed the change? If I operate it correctly I don't see any
change in the trac846 branch.

comment:5 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 in reply to: ↑ 3 Changed 9 years ago by jinmei

Replying to 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.

comment:7 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:8 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Yes, you're right about the code being needlessly complicated, I just fixed the counting that seemed wrong without thinking about the whole function.

I'd use your first proposal, that one seems shortest and simplest to read. Is it OK to merge now?

Thanks

comment:9 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:10 in reply to: ↑ 8 Changed 9 years ago by jinmei

Replying to vorner:

Yes, you're right about the code being needlessly complicated, I just fixed the counting that seemed wrong without thinking about the whole function.

I'd use your first proposal, that one seems shortest and simplest to read. Is it OK to merge now?

Yes, go ahead.

comment:11 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 Changed 9 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed

Ok, thanks, merged.

Note: See TracTickets for help on using tickets.