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

fix ignore.ignore_private_members=false being interpreted as =true #31

Closed
wants to merge 1 commit into from

Conversation

strager
Copy link

@strager strager commented Oct 3, 2022

If the user specifies 'ignore.ignore_private_members=false' in their .hdoc.toml, we are interpreting that as if they wrote 'ignore.ignore_private_members=true'. This is because .as_boolean returns a pointer, and we are casting that non-null pointer to a boolean, which always yields true.

Fix the problem:

  • Avoid auto, which made the problem harder to see
  • Use .value() (which returns a std::optional) instead of .as_boolean() (which returns a toml::value*)
  • Use * to extract the value from the std::optional (which the compiler helpfully now tells us to do)

If the user specifies 'ignore.ignore_private_members=false' in their
.hdoc.toml, we are interpreting that as if they wrote
'ignore.ignore_private_members=true'. This is because .as_boolean
returns a pointer, and we are casting that non-null pointer to a
boolean, which always yields true.

Fix the problem:

* Avoid auto, which made the problem harder to see
* Use .value<bool>() (which returns a std::optional<bool>) instead of
  .as_boolean() (which returns a toml::value<bool>*)
* Use * to extract the value from the std::optional<bool> (which the
  compiler helpfully now tells us to do)
@hdoc
Copy link
Owner

hdoc commented Oct 3, 2022

Good catch! There is actually a unit test for this, but the test doesn't catch this bug since the default value of cfg.ignorePrivateMembers is false and is only overwrriten if the entry exists in the TOML. Since it's unusual that someone would set the value in the TOML to the default (thereby triggering this bug), we didn't see it prior to your report. Thank you!

We will need you to sign a Contributor Assignment Agreement (CAA) and email it to cla@hdoc.io so that this can be merged into our repository. This is unfortunately necessary for legal reasons. I have attached the CAA to this comment.

@strager
Copy link
Author

strager commented Oct 3, 2022

I have attached the CAA to this comment.

I don't see the attachment on GitHub (#31 (comment)) or in my email.

@hdoc
Copy link
Owner

hdoc commented Oct 3, 2022

I apologize, I mistakenly omitted it. The CAA is attached to this comment.

hdoc-individual-caa.pdf

@hdoc
Copy link
Owner

hdoc commented Oct 7, 2022

Hello, just checking in to see if you have had the chance to review the CAA and send it to us. We would like to integrate this change in our next release but we need your CAA to do that. Please let us know if there's anything we can do to help. Thank you!

@strager
Copy link
Author

strager commented Oct 9, 2022

In the document, who is "hdoc"? Is this a business? Where is the business registered?

@hdoc
Copy link
Owner

hdoc commented Oct 9, 2022

The hdoc IP is owned by a Canadian corporation and it is the counterparty in the CAA.

@strager
Copy link
Author

strager commented Oct 9, 2022

The hdoc IP is owned by a Canadian corporation

Which Canadian corporation? I can't sign a contract if I don't know who I'm signing it with.

@hdoc
Copy link
Owner

hdoc commented Oct 9, 2022

[redacted for privacy reasons]

@hdoc
Copy link
Owner

hdoc commented Oct 10, 2022

Are you still interested in merging this contribution?

@hdoc hdoc closed this Oct 17, 2022
@hdoc
Copy link
Owner

hdoc commented Oct 25, 2022

@strager, this was fixed in hdoc 1.4.0 which was just released. Though we implemented the fix in a different method than your PR, we have still credited you with reporting the bug in the release notes, which can be found here: https://hdoc.io/release-notes/#version-1-4-0-24-october-2022. Thank you for reporting this bug!

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.

2 participants