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

add setting unicast bit #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XiXiangFiles
Copy link

Description:

for the unicast bit issue

I just rebase to the latest. previous pr: #49

related issues

#21

@codecov-commenter
Copy link

Codecov Report

Merging #96 (ab17088) into master (7b66620) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   97.99%   98.00%   +0.01%     
==========================================
  Files           6        6              
  Lines        2046     2057      +11     
==========================================
+ Hits         2005     2016      +11     
  Misses         41       41              
Files Coverage Δ
index.js 98.08% <100.00%> (+0.01%) ⬆️

@silverwind
Copy link
Collaborator

See #94 (comment)

@XiXiangFiles
Copy link
Author

@silverwind thx for your reviewing. I modify it to qu from QU. Thank you for continuing to leave comments on the Pull Request from many years ago

Copy link

@Kleclerc18 Kleclerc18 left a comment

Choose a reason for hiding this comment

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

So thankful

} else {
q.class = classes.toString(classValue)
}

offset += 2

const qu = !!(q.class & QU_MASK)

Choose a reason for hiding this comment

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

I think this section is still problematic (and there doesn't seem to be a test for decoding a question with the QU bit set) - if the bit is set, then we end up attempting a bitwise AND operation on the q.class string value (in line 1648 just below), which wipes out the string value (the result is 0).

Thank you very much for this effort - would you mind also adding a test for the decode function?

Copy link
Author

Choose a reason for hiding this comment

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

Certainly, this change was quite some time ago. I recall this PR request occurred in 2018 or 2019. This weekend, I will take a look and make some adjustments.

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.

None yet

5 participants