Opened 10 years ago

Closed 9 years ago

#200 closed enhancement (fixed)

use new Name::split() in data source

Reported by: jinmei Owned by: jinmei
Priority: low Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: data source 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

See trac #49.

This was not originally planned as a goal for the 2nd release, but should be pretty easy. So I'd consider this as one of the possible features for that release with a lower priority.

Subtickets

Attachments (1)

split.diff (2.8 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by shane

  • Milestone changed from 04. 2nd Incremental Release: Early Adopters to 05. 3rd Incremental Release

Changed 9 years ago by jinmei

comment:2 Changed 9 years ago by jinmei

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

Please review the attached patch. Very short, and should be trivial.

comment:3 Changed 9 years ago by jinmei

  • Type changed from defect to enhancement

comment:4 Changed 9 years ago by shane

  • Owner changed from UnAssigned to each

Hey Evan, can you have a look? While the changes look simple to me, I don't understand the motivation and suchlike so would prefer someone who did review these. Thanks!

comment:5 follow-up: Changed 9 years ago by each

  • Owner changed from each to jinmei

for (int i = diff - 1; i >= 0; --i) {

const Name sub(task->qname.split(i));

I'm not sure, but I think maybe the ">= 0" should be "> 0". As written, I think this would cause it to match down all the way to the qname itself, whereas we actually want it to stop matching at the parent.

Also, I think you can eliminate the "nlen" variable now.

Otherwise the patch looks fine.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by jinmei

Replying to each:

 	        for (int i = diff - 1; i >= 0; --i) { 
 	            const Name sub(task->qname.split(i));

I'm not sure, but I think maybe the ">= 0" should be "> 0". As written, I think this would cause it to match down all the way to the qname itself, whereas we actually want it to stop matching at the parent.

You're right, good catch.

I've added a unit test case to catch the bug, and left a note as a comment that we exclude the apex and the qname. (frankly, I think the matching rule here is quite tricky and error prone, and I hope we can eventually refactor the logic. but that's certainly another topic).

Also, I think you can eliminate the "nlen" variable now.

Good suggestion, applied.

Otherwise the patch looks fine.

I've now created a shortlived branch (branches/trac200) including everything and committed the changes there. It's single svn changeset, r2157. Could you confirm that?

Oh, and I've realized I didn't include expected changelog entry. Here it is:

  57.?	[func]		jinmei
	lib/datasrc: used a simpler version of Name::split (change 31) for
	better readability.  No behavior change. (Trac #200, svn TODO)

Thanks,

comment:7 Changed 9 years ago by jinmei

  • Owner changed from jinmei to each

comment:8 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by each

  • Owner changed from each to jinmei

Replying to jinmei:

I've now created a shortlived branch (branches/trac200) including everything and committed the changes there. It's single svn changeset, r2157. Could you confirm that?

Minor grammatical error in a comment, "shouldn't confusing delegation processing" should read "shouldn't confuse".

Everything else looks fine, go ahead and commit.

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

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

Replying to each:

Replying to jinmei:

I've now created a shortlived branch (branches/trac200) including everything and committed the changes there. It's single svn changeset, r2157. Could you confirm that?

Minor grammatical error in a comment, "shouldn't confusing delegation processing" should read "shouldn't confuse".

Oh, yet another good catch. Thanks. Fixed and committed. Closing.

Note: See TracTickets for help on using tickets.