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.

Sign up for GitHub

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

Removes dependence on boost::dynamic_bitset and fixes test failure on Windows #108

Merged
merged 3 commits into from
Mar 27, 2016
Merged

Removes dependence on boost::dynamic_bitset and fixes test failure on Windows #108

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e1ebb8
Removes dependence on boost::dynamic_bitset and fixes #107 (test fail…
sherm1 Mar 26, 2016
5d07547
bitset template argument is a size_t
sherm1 Mar 27, 2016
33afc0f
Fix specified-size int types. Shouldn't be "fast" because those can b…
sherm1 Mar 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/fcl/data_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ namespace fcl
{

typedef double FCL_REAL;
typedef std::uint_fast64_t FCL_INT64;
typedef std::int_fast64_t FCL_UINT64;
typedef std::uint_fast32_t FCL_UINT32;
typedef std::int_fast32_t FCL_INT32;
typedef std::int64_t FCL_INT64;
typedef std::uint64_t FCL_UINT64;
typedef std::int32_t FCL_INT32;
typedef std::uint32_t FCL_UINT32;
Copy link
Member

Choose a reason for hiding this comment

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

Good thing you caught the backwards definition! That was my bad. According to http://en.cppreference.com/w/cpp/types/integer std::int64_t et al. are an optional part of the C++11 standard and are thus not guaranteed to be defined, whereas the fast versions are. What problems occur if there are too many bits?

Copy link
Member Author

Choose a reason for hiding this comment

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

std::int64_t et al. are an optional part of the C++11 standard ...

They are only optional on platforms that can't provide an exactly 32 & 64 bit 2's complement type. According to C++11 7.20.1.1/3 (quoting from stackoverflow):

However, if an implementation provides integer types with widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a two’s complement representation, it shall define the corresponding typedef names.

A more general solution would be to use the "least" types like std::int_least32_t which will be exactly the specified size if it is available, otherwise the next larger size. The problem with that IMO is that to me the 32 in FCL_INT32 implies exactly 32 bits by analogy with int32_t. I would rename it FCL_INT_LEAST32 if that was what it meant. Actually I would prefer just to remove these macros altogether and use the standard typedefs instead (in another PR).

What problems occur if there are too many bits?

That would depend on the usage. I believe in the Morton case, the 32 & 64 bit specializations were intended to provide compact storage, so choosing a 64 bit representation for the 32 would be odd (even if that were the fastest type). I would just like the integer size dependencies to be clear. Currently this is kind of a mess in FCL and I'm wondering whether that may account for some of the failures I'm seeing on Windows.

Here are some related discussions: Google style guide, Khronos issue

Copy link
Member

Choose a reason for hiding this comment

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

What a mess indeed. I am satisfied that your suggested change is the best (or least bad) choice.


/// @brief Triangle with 3 indices for points
class Triangle
Expand Down