BIND 10 #2432: define and implement base and libdns++ version of RRsetCollection

[BIND 10 Development]

#2432: define and implement base and libdns++ version of RRsetCollection
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  libdns++      |                    Milestone:
            Keywords:                |  Sprint-20130108
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  5             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hmm, I missed quite some amount of discussion over the Christmass O:-).

 '''General'''

 For the iterator, does boost FOREACH work with the current one? Does it
 work
 with the lower-case one? I think it might be worth an exception to the
 style
 guide if we got working FOREACH and possibly some more std algoritms. But,
 of
 course, documented why the style is broken.

 For the collation, I think there could be a private `mergeRRset` which
 would
 add a new `RRset` or merge it with the existing one. It could be used in
 the
 initial loading only and the `addRRset` would stay the same. Then the
 result of
 find could never change when observed from outside. But that's suggestion
 of
 the ticket to be.

 '''Code'''

 Is the explicit template instantiation needed here? Wouldn't it be more
 readable without it?
 {{{#!c++
 constructHelper<const char*>(filename, origin, rrclass);
 }}}

 Maybe this should be in explicit `\throw`?
 {{{#!c++
 /// stream. The constructor throws MasterLoaderError if there is an
 /// error during loading.
 }}}

 Is `\returns` correct doxygen keyword? I've seen `\return` in our code,
 but I
 don't remember seeing the third form.

 If the `collection2` would be shorter here, it'd try to dereference an end
 iterator, which probably wouldn't work. May I suggest using `ASSERT_TRUE`
 here,
 so the test doesn't crash but is aborted instead if this happens?
 {{{#!c++
     while (iter != collection.end()) {
          EXPECT_TRUE(iter2 != collection2.end());
          EXPECT_EQ((*iter).toText(), (*iter2).toText());
          ++iter;
          ++iter2;
     }
 }}}

 Is the `exists` variable needed? Can't the method be called directly from
 `EXPECT_TRUE`?
 {{{#!c++
     bool exists = collection.removeRRset(Name("foo.example.org"),
                                          rrclass, RRType::A());
     EXPECT_TRUE(exists);
 }}}

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