Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS Update Misc #154

Merged
merged 5 commits into from
Nov 12, 2014
Merged

DNS Update Misc #154

merged 5 commits into from
Nov 12, 2014

Conversation

andrewtj
Copy link
Collaborator

71436da needs a closer look since I just took a guess and the tests passed.

104b206 makes RemoveRRset() work as intended, though I think those update functions shouldn't actually behave the way they do. It makes sense for SetQuestion() to clear the question section, but I don't think the prerequisite and update helpers should clear they're respective sections as stacking a series of prerequisites and changes is likely common.

}
u.Ns = make([]RR, 0, len(m))
for t := range m {
u.Ns = append(u.Ns, &ANY{t})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the 2-loop approach? You can just as easily wrap for for t := look in the for _, r := range rr one, no?
Also can the copy() function be of use here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep the second loop could be dropped and replaced with a presence check of the map. copyHeader() should shave off a few lines.

miekg added a commit that referenced this pull request Nov 12, 2014
@miekg miekg merged commit f3ed7c2 into miekg:master Nov 12, 2014
@andrewtj andrewtj deleted the atj-update-misc branch November 12, 2014 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants