Skip to content

New boolean representation#637

Closed
achilleas-k wants to merge 10 commits intoG-Node:masterfrom
achilleas-k:boolenum_
Closed

New boolean representation#637
achilleas-k wants to merge 10 commits intoG-Node:masterfrom
achilleas-k:boolenum_

Conversation

@achilleas-k
Copy link
Member

Booleans are now stored as enums with members "FALSE" and "TRUE".

Fixes issue #635.

}

void check_bool_enum(h5x::DataType type) {
assert(type.member_count() == 2);
Copy link
Member

Choose a reason for hiding this comment

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

assert statements can be turned into NOOPs with the right define (NDEBUG), which is not a good idea. Throwing an exception, or returning DataType::Nothing.

Copy link
Member

Choose a reason for hiding this comment

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

On a second though, you should be able to use boolfiletype.enum_equal(type) for the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used assert since it's also used in data_type_from_h5 when the the vclass is unknown. It's also used when the member count in the compound data type is wrong.

Should those cases be throwing exceptions as well?

Copy link
Member

Choose a reason for hiding this comment

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

Assert is to find programming errors. In general, if we didn't fuck up, then there should never be a case where any of the asserts are it. If for whatever reason that happens then in data_type_from_h5 we return a DataType::Nothing, which is one of the sensible things to do. Throwing an exception would also work, but I guess the user should never actually see it.
In the bool case if the asserts are "not working" then we would accept something not a bool as a bool which we really shouldn't do.

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