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.

Sign up for GitHub

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

Jump to bottom

Dns server cleanup #5194

Merged
merged 5 commits into from
Oct 4, 2018
Merged

Dns server cleanup #5194

merged 5 commits into from
Oct 4, 2018

Conversation

Makuna
Copy link
Collaborator

@Makuna Makuna commented Sep 30, 2018

No description provided.

@Makuna
Cleanup
644ed7a 
Removed member variables that are not required outside a member call lifetime
@Makuna
add destructor
1887ec6
@Makuna
Use size_t and test it
4ee3fd6
@Makuna Makuna mentioned this pull request Sep 30, 2018
Closed
@earlephilhower
Copy link
Collaborator

Just a note, @Makuna, you can keep a PR open and just git push onto your private branch and it will update the PR with your changes. Not a big deal here, but when it's a long set of changes with comments using the same PR makes it way easier to follow any comments.

@Makuna
Copy link
Collaborator Author

Makuna commented Sep 30, 2018

@earlephilhower I tried that, but the PR would not update. Not sure what was going on; but it would not allow to re-run the tests and the update branch was greyed out.

@Makuna
Copy link
Collaborator Author

Makuna commented Sep 30, 2018

Also, Since I am the author of this change, I feel other collaborators should review and merge. Is that the normal policy?

earlephilhower
earlephilhower approved these changes Oct 4, 2018
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

libraries/DNSServer/src/DNSServer.cpp
// name size cold be easily contained without a reallocation
// - max size would be 253, in 2013, average is 11 and max was 42
//
parsedDomainName.reserve(32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay! String::reserve() examples are few and far between.

libraries/DNSServer/src/DNSServer.h
@@ -6,6 +6,8 @@
#define DNS_QR_RESPONSE 1
#define DNS_OPCODE_QUERY 0

#define MAX_DNSNAME_LENGTH 253

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not used anywhere as far as I can see, but shouldn't do any harm.

@earlephilhower earlephilhower merged commit 656bf14 into master Oct 4, 2018
@Makuna Makuna deleted the DnsServerCleanup branch October 14, 2018 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Makuna @earlephilhower @devyte