Replace redundant packet type check in handler with assert.

[yurivict]

This change is 

[iphydf]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

toxcore/DHT.c, line 2538 at r1 (raw file):

    DHT *dht = (DHT *)object;

    if (packet[0] == NET_PACKET_CRYPTO) {

How about:

if (packet[0] != NET_PACKET_CRYPTO) {
    assert(!"unexpected packet kind");
    return 1;
}

The check is redundant, but removing it makes the function's correctness non-local. It's a function precondition, and we currently don't have a good way to express those. Perhaps add a // TODO comment saying that function preconditions should be expressed better (e.g. with an assert-like macro we define for that purpose).

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[yurivict]

This is a very shallow check, it merely checks that the handler is plugged at the right slot, which it obviously is. Also no other handlers perform such check. I suggest it is deleted.

[iphydf]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

toxcore/DHT.c, line 2538 at r1 (raw file):

Previously, iphydf wrote…

How about:

if (packet[0] != NET_PACKET_CRYPTO) {
    assert(!"unexpected packet kind");
    return 1;
}

The check is redundant, but removing it makes the function's correctness non-local. It's a function precondition, and we currently don't have a good way to express those. Perhaps add a // TODO comment saying that function preconditions should be expressed better (e.g. with an assert-like macro we define for that purpose).

Can you just turn it into an assert then? "which it obviously is" is not a good criterion to remove a check. The fact that it's not checked elsewhere probably means we should check it there as well. It is a function precondition, because it won't work correctly if the handler was plugged into the wrong slot. The problem with not checking it at all is that if somehow we end up changing some code that ends up putting it into the wrong slot, it might fail silently doing some incorrect thing.

---

Comments from Reviewable

[yurivict]

assert(packet[0] == NET_PACKET_CRYPTO); might be better.

[iphydf]

Yes, that's what I meant.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[iphydf]

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[iphydf]

Can you enable the checkbox that lets collaborators push to your PR branch? That way, I can rebase it on master and then merge after CI completes.

[iphydf]

Please rebase on master.

[piling]

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[iphydf]

@yurivict can you rebase this on master and enable the checkbox (on the right) to allow us to rebase it again if necessary?

[iphydf]

@yurivict also squash the two commits in one, and give it an appropriate title.

[iphydf]

@yurivict I'd like to push this before tomorrow. Will you have time to either click the checkbox on the right, or squash and rebase these commits on master?

[yurivict]

I am sorry I have been busy lately to engage in this. Where is the button you talk about?

[dvor]

@yurivict I guess he is talking about this checkmark

[yurivict]

It doesn't show such button on my screen:

[yurivict]

It says "this pull request can be merged by project collaborators". Why don't you just do this?

[dvor]

@yurivict we want to update pull request with required changes before merging it.

Try scrolling to the top of the page. There should be block as on my screenshot where you will find needed checkmark.

[yurivict]

Ok, found it.

[iphydf]

"This pull request can be merged" is a lie. If I try, it actually doesn't work. That's a github bug.

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable