Page MenuHomePhabricator

Add constants to dns.h
ClosedPublic

Authored by nakihito on Mon, Mar 9, 22:44.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC0da65ecee208: Add constants to dns.h
Summary

Moves some constants from seeder/dns_tests.cpp to dns.h.

Test Plan
ninja
ninja check-bitcoin-seeder

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Mon, Mar 9, 22:44
Owners added a reviewer: Restricted Owners Package.Mon, Mar 9, 22:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 9, 22:44
Fabien requested changes to this revision.Tue, Mar 10, 08:02
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/seeder/dns.cpp
77 ↗(On Diff #16859)

Please double check that this won't cause issues later...

85 ↗(On Diff #16859)

Please mention the null character so the -3 makes sense

101 ↗(On Diff #16859)

... here for example

142 ↗(On Diff #16859)

Now a <260 chars>.pwned name will be fine.

src/seeder/dns.h
14 ↗(On Diff #16859)

I think what you want here is constexpr

This revision now requires changes to proceed.Tue, Mar 10, 08:02
nakihito updated this revision to Diff 16888.Wed, Mar 11, 23:21

Addressed critiques.

Fabien requested changes to this revision.Thu, Mar 12, 07:59
Fabien added inline comments.
src/seeder/dns.cpp
77 ↗(On Diff #16888)

Whatever the type is, if you go from signed to unsigned you need to be sure that it is safe when using the - operator (which is done a few lines below) because this is a behavior change.
What is the rational behind this change (which seems unrelated to what the summary describe as the intent for this diff) ?

130 ↗(On Diff #16888)

Mind changing k to ok while you're at it ?

142 ↗(On Diff #16888)

There is a type for pointer arithmetic: https://en.cppreference.com/w/cpp/types/ptrdiff_t

src/seeder/dns.h
14 ↗(On Diff #16888)

2 questions:

  • Is the static keyword necessary ?
  • Why not make them all constexpr ?
This revision now requires changes to proceed.Thu, Mar 12, 07:59
nakihito updated this revision to Diff 16907.Thu, Mar 12, 20:20

Removed static and changed const to constexpr. Also undid changes to variables in parse_name() and write_name(). Changed MAX_LABEL_LENGTH from size_t to int.

nakihito added inline comments.Thu, Mar 12, 20:21
src/seeder/dns.cpp
77 ↗(On Diff #16888)

These changes were made to answer some type comparison mismatch compiler warnings. I have determined the less intrusive change is to change the typing of the constants instead.

142 ↗(On Diff #16888)

I'll leave this for a different diff.

Fabien accepted this revision.Fri, Mar 13, 07:30
This revision is now accepted and ready to land.Fri, Mar 13, 07:30
This revision was automatically updated to reflect the committed changes.