Skip to content

Conversation

@david-cermak
Copy link
Collaborator

@david-cermak david-cermak commented Nov 27, 2025

unlike discoveries, where they need to stay in additional section
Quick fix of regression b7b8c5d

@david-cermak
Copy link
Collaborator Author

@tanyanquan PTAL, too

send_flush, false)) {
return false;
}
} else if (is_any_instance_question) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can put this block first, followed by
else if (question->type == MDNS_TYPE_PTR || question->type == MDNS_TYPE_ANY)
for clarity

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using the original expression (question->type == MDNS_TYPE_ANY) && is_instance_question , just to follow consistency with other if conditions, and adding a comment in this if branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, have swapped these branches and removed the unnecessary is_any_instance_question.
thanks!

Copy link
Collaborator

@zwx1995esp zwx1995esp left a comment

Choose a reason for hiding this comment

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

LGTM, 👍

send_flush, false)) {
return false;
}
} else if (is_any_instance_question) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using the original expression (question->type == MDNS_TYPE_ANY) && is_instance_question , just to follow consistency with other if conditions, and adding a comment in this if branch?

unlike discoveries, where they need to stay in additional section
Quick fix of regression b7b8c5d
Also adds a test to check the answer section for queries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants