Opened 9 years ago

Closed 9 years ago

#358 closed task (fixed)

more cleanup for the message class

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month 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

This is a next round of DNS message class cleanups. I plan to change header flags and message sections from separete user defined classes to enumerates, considering the balance between type safety and usability. I also add more detailed tests and documentation for these.

Subtickets

Change History (6)

comment:1 Changed 9 years ago by jinmei

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

branches/trac358 is ready for review. Branch point is r3109.

Like previous sets of cleanups, diff is large but should mostly be trivial replacements.

There's one unrelated change: signature change to Message::hasRRset(). I noticed the previous signature (passing an RRset) wasn't well defined while I tried to write a test for this method wrt the main change of this branch, and decided to revise the signature using this opportunity.

There are still some not or poorly documented part of the Message class. And there are still missing tests. I plan to address these in the next (which will be hopefully the final) set of cleanup.

This is the proposed changelog entry:

  103.	[func]*		jinmei
	src/lib/dns: Changed DNS message flags and section names from
	separate classes to simpler enums, considering the balance between
	type safety and usability.  API has been changed accordingly.
	More documentation and tests were provided with these changes.
	(Trac #358, rTBD)

comment:2 Changed 9 years ago by jinmei

I made a few more changes assuming no one had started review (r3130).

comment:3 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

The code seems ok and IMO can be merged.

There's just one think I'd like to propose. The enums, if numbers are not assigned explicitly, start at 0 and increase by 1 for each following value. I saw having a MAX_<whatever> value as the last one and used the way you use NUM_SECTIONS. It has the nice property of updating the count automatically when new value is added (it is really not probable that the number of sections would change, thought). You can implement it if you like it or keep it as it is.

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

Replying to vorner:

The code seems ok and IMO can be merged.

There's just one think I'd like to propose. The enums, if numbers are not assigned explicitly, start at 0 and increase by 1 for each following value. I saw having a MAX_<whatever> value as the last one and used the way you use NUM_SECTIONS. It has the nice property of updating the count automatically when new value is added (it is really not probable that the number of sections would change, thought). You can implement it if you like it or keep it as it is.

Thanks for the review. This is indeed a good suggestion, but after thinking over it I've decided not to use the default assignments at the moment. Right now, the code internally uses the enum values as vector indices (which is itself controversial usage and may be revisited), so I think it's safer to keep the explicit assignment.

comment:6 Changed 9 years ago by jinmei

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

merged to trunk, closing ticket.

Note: See TracTickets for help on using tickets.