Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#463 closed task (complete)

"glue OK" mode for (Memory)Zone::find()

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: y2 12 month milestone
Component: data source 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 another A-team sprint task. Description copied from wiki:

This is a continuation of #454, and is necessary to complete the additional NS handling (#453). We add an option flag to find(). If it specifies the "glue OK" mode (which is off by default), find() doesn't stop the search at a zone cut and tries to find the longest match, if any.

Subtickets

Attachments (1)

glue-ok.diff (12.3 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 follow-up: Changed 9 years ago by jinmei

I've implemented this feature on top of a snapshot of branches/trac454.

It's ready for review. Once #454 is completed, I'll create a branch and put it to the review queue. In the mean time I'm attaching the current diff.

Changed 9 years ago by jinmei

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

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

Replying to jinmei:

I've implemented this feature on top of a snapshot of branches/trac454.

It's ready for review. Once #454 is completed, I'll create a branch and put it to the review queue.

Branch created: branches/trac463. It's ready for review.

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

Just one thing:

return ((state->options_ & FIND_GLUE_OK) != 0 ? false : true)

This seems unnecesarilly complicated. Wouldn't return(!(state->options_ & FIND_GLUE_OK)); be better? The only thing that the condition does is negation.

Otherwise it seems to be OK to merge.

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

  • Owner changed from jinmei to vorner

Replying to vorner:

Just one thing:

return ((state->options_ & FIND_GLUE_OK) != 0 ? false : true)

This seems unnecesarilly complicated. Wouldn't return(!(state->options_ & FIND_GLUE_OK)); be better? The only thing that the condition does is negation.

Otherwise it seems to be OK to merge.

I admit the current notaion doesn't look nice. But I'm not very happy with '!(state->options_ & FIND_GLUEOK)' because it assumes implicit conversion from integer to bool. This may be a pedantic concern in practice, but also follows BIND 9's coding guideline (some of which I adopt): http://bind10.isc.org/wiki/BIND9CodingGuidelines

In any case the original code should have been simplified. How about changing it as follows?

            // Unless glue is allowed the search stops here, so we return
            // false; otherwise return true to continue the search.
            return ((state->options_ & FIND_GLUE_OK) == 0);

(I added some comments to clarify the intent)

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

Replying to jinmei:

In any case the original code should have been simplified. How about changing it as follows?

            // Unless glue is allowed the search stops here, so we return
            // false; otherwise return true to continue the search.
            return ((state->options_ & FIND_GLUE_OK) == 0);

(I added some comments to clarify the intent)

I've committed this proposed change to the branch (r4107), and merged the branch to trunk to help #453 move forward.

I'm keeping the ticket still open. Please let me know if this change is okay or not.

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

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

Yes, this seems OK. Let's close it, then.

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

Replying to vorner:

Yes, this seems OK. Let's close it, then.

Okay, thanks.

Note: See TracTickets for help on using tickets.