Opened 10 years ago

Closed 10 years ago

#95 closed task (fixed)

review: a minor fix to the name module

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: 03. 1st Incremental Release
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

I've made a small change to src/lib/dns/name.cc (which was once reviewed) to address a bug. The additional fix should be reviewed.

It's a pretty small fix and should be easily reviewed and confirmed.

The relevant changeset is http://bind10.isc.org/changeset/1134

I'm assigning the ticket to Shane as the review dispatcher.

Subtickets

Change History (2)

comment:1 follow-up: Changed 10 years ago by shane

  • Owner changed from shane to jinmei
  • Status changed from new to assigned

This looks okay, although I have a couple of minor points.

First, I'm not sure why we don't we use <ctypes.h> and the tolower() function for lowercasing. Presumably there are portability and/or performance reasons for this. I'm not complaining, just curious.

Second, I would add a few comments to the tables. For the digitvalue table:

0, 1, 2, 3, 4, 5, 6, 7, 8, 9, -1, -1, -1, -1, -1, -1, '0' to '9'

-1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1, 'A' to 'F'
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1,
'a' to 'f'

For the maptolower table:

0x40, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, ..., 'A'-'G'
0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f,
'H'-'O'
0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, 'P'-'W'
0x78, 0x79, 0x7a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f,
'X'-'Z', ...

Just so it's obvious what is going on. Maybe?

Note that these comments are not required, so I approve these changes "as is", if you decide they are not helpful.

comment:2 in reply to: ↑ 1 Changed 10 years ago by jinmei

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

Replying to shane:

First, I'm not sure why we don't we use <ctypes.h> and the tolower() function for lowercasing. Presumably there are portability and/or performance reasons for this. I'm not complaining, just curious.

Good point. This part was a verbatim copy of BIND 9, and I was also wondering why they used a handmade array and whether we could simply use tolower(). In fact, I just used towlower() in rrparamregistry.cc, so the design choice is not consistent.

We should probably revisit this design, probably after benchmarking the performance. For now, I left the code as is and added a comment about this point. (r1488).

Second, I would add a few comments to the tables. For the digitvalue table:

For the maptolower table:

0x40, 0x61, 0x62, 0x63, 0x64, 0x65, 0x66, 0x67, ..., 'A'-'G'
0x68, 0x69, 0x6a, 0x6b, 0x6c, 0x6d, 0x6e, 0x6f,
'H'-'O'
0x70, 0x71, 0x72, 0x73, 0x74, 0x75, 0x76, 0x77, 'P'-'W'
0x78, 0x79, 0x7a, 0x5b, 0x5c, 0x5d, 0x5e, 0x5f,
'X'-'Z', ...

Just so it's obvious what is going on. Maybe?

Okay, added in r1487.

Note that these comments are not required, so I approve these changes "as is", if you decide they are not helpful.

Thanks for the review. I'll close this ticket.

Note: See TracTickets for help on using tickets.