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

Potentially Confusing Behavior of hasMasksSet #28837

Open
p0fi opened this issue Aug 23, 2023 · 2 comments
Open

Potentially Confusing Behavior of hasMasksSet #28837

p0fi opened this issue Aug 23, 2023 · 2 comments

Comments

@p0fi
Copy link
Contributor

p0fi commented Aug 23, 2023

📖 Description

While #27253 aligned the behavior of hasMasksSet between the python and c++ implementation its behavior seems a bit confusing.

Currently hasMasksSet[0x03] would yield true on 0b0000 0001 AND on 0b0000 0010 since it returns true if any of the bits is set. The semantics of the method would suggest it would only yield true if all bits in the mask are set.

The behavior which is currently implemented would be more suitable for a method with the name hasAtLeastOneMaskSet while hasMasksSet should only return true if all bits are set.

💁‍♂️ Suggestion

  1. Add new method hasAtLeastOneMaskSet which implements the current behavior of hasMasksSet
  2. Change implementation of hasMasksSet to only return true if all bits in the mask are set
@vivien-apple
Copy link
Contributor

I am not against changing the name as I agree it does not state clearly what it does.
That said if you expect a well defined value one can just use value no ?

@bzbarsky-apple
Copy link
Contributor

Fundamentally, it seems like we should do one of two things:

  1. Disallow non-single-bit values in the array.
  2. Have sane semantics for non-single-bit values (which should be what?) and have the name clearly express them.

We currently do not seem to have a good way other than using hasMasksSet to test for "at least one of these bits is set". But renaming hasMasksSet to hasAtLeastOneMaskSet would also not be correct, since hasMasksSet: [0x1, 0x2] does in fact require both bits to be set, right? It would need to be something like hasAtLeastOneBitFromEachMaskSet or something.

Maybe for now if we have no use cases for the "at least one bit is set" we should just take option 1..... and then if we have such use cases arise we add a different function to address them.

This seems to affect the following tests:

  • Test_TC_ACFREMON_1_1.yaml
  • TestConstraints.yaml
  • Test_TC_CDOCONC_1_1.yaml
  • Test_TC_CMOCONC_1_1.yaml
  • Test_TC_FLDCONC_1_1.yaml
  • Test_TC_HEPAFREMON_1_1.yaml
  • Test_TC_NDOCONC_1_1.yaml
  • Test_TC_OZCONC_1_1.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants