Opened 9 years ago

Closed 9 years ago

#410 closed defect (fixed)

RRset::getRdataIterator returns object in invalid state

Reported by: vorner Owned by: jinmei
Priority: low Milestone:
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Documentation of the iterator clearly states that af isLast() returs true, the iterator should not be used. However, it does not say that newly born iterator should not be used before call to first() as well and it is counter-intuitive. I spent few hours trying to figure why it crashes. When the iterator is created, the it_ field (the wrapped iterator) is undefined:

BasicRdataIterator(const std::vector<rdata::ConstRdataPtr>& datavector) :
        datavector_(&datavector) {}

I suggest that either the iterator returned should point to the first item or the documentation should equally clearly say that first() must be called before use.

Subtickets

Change History (6)

comment:1 Changed 9 years ago by shane

What's normal iterator behavior? I would think that it would point to the first item when constructed, unless there is a performance reason not to do this (which seems a bit strange, but I can imagine that it is possible).

comment:2 Changed 9 years ago by vorner

Well, this iterator is java-like one (for reasons described in documentation, creating C++-like one would be hard if it was to be abstract class and copyable), so it already acts different than normal C++ iterators. But as java iterator does not have the first() method, it must point to the first item when it is created, so it does not act fully as java one either.

I too thought it would point to the first element, before reading the code and realizing it points to invalid position. That's what I mean by counter-intuitive.

And I doubt the performance reasons, now one more virtual call must happen when initializing it (it is useless otherwise). If it was called from the constructor, it would be called non-virtually and should be faster than this situation.

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

Replying to vorner:

I suggest that either the iterator returned should point to the first item or the documentation should equally clearly say that first() must be called before use.

Hmm, I don't remember why I left the initialized position of the iterator cursor undefined, but I can't think of a reason why not now, and I'd agree it makes more sense to initialize it with the "first" position.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing

addressed this issue in branches/trac410. the changeset is r3529.

The only important change is that for the BasicRdataIterator constructor (defined in rrset.cc). Others are minor cleanups and documentation update.

This is the proposed changelog entry:

  117.	[func]		jinmei
	src/lib/dns: changed the interface of AbstractRRset::getRdataIterator()
	so that the internal cursor would point to the first RDATA
	automatically.  This will be a more intuitive and less error prone
	behavior.  This is a backward compatible change. (Trac #410, rTBD)

(I chose 'func' instead of 'bug' because this didn't really cause a failure with existing code or didn't behave against the documentation.)

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to jinmei

Thanks for the change, I think it can be merged.

comment:6 Changed 9 years ago by jinmei

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

committed, closing.

Note: See TracTickets for help on using tickets.