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

Cmake namespace #136

Merged
merged 12 commits into from
Mar 5, 2021
Merged

Cmake namespace #136

merged 12 commits into from
Mar 5, 2021

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Mar 4, 2021

Add a means to change the namespace the units library lives under.
See Issue #110

It can be set using CMAKE with -DUNITS_NAMESPACE=<desired namespace>

or in the code

#define UNITS_NAMESPACE <desired>   

if nothing is done the namespace defaults to units

@codecov-io
Copy link

codecov-io commented Mar 5, 2021

Codecov Report

Merging #136 (65838c8) into master (7623c10) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #136   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files           6        6           
  Lines        4780     4790   +10     
=======================================
+ Hits         4753     4763   +10     
  Misses         27       27           

Copy link
Contributor

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

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

As far as I can see, no changes to the unit tests were made, i.e., test cannot be built unless UNITS_NAMESPACE=units. Intentional?

If not, SimonHeybrock@0d9d7ae has the required modifications.

units/CMakeLists.txt Show resolved Hide resolved
constexpr int32_t mole{2};
constexpr int32_t radian{3};
constexpr int32_t currency{2};
constexpr int32_t count{2};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 more readable than what I had.

phlptp and others added 3 commits March 5, 2021 13:39
@phlptp phlptp merged commit 7330568 into master Mar 5, 2021
@phlptp phlptp deleted the cmake_namespace branch March 5, 2021 23:31
Comment on lines 105 to 112
meter_(maxNeg(bitwidth::meter)), second_(maxNeg(bitwidth::second)),
kilogram_(maxNeg(bitwidth::kilogram)),
ampere_(maxNeg(bitwidth::ampere)),
candela_(maxNeg(bitwidth::candela)),
kelvin_(maxNeg(bitwidth::kelvin)), mole_(maxNeg(bitwidth::mole)),
radians_(maxNeg(bitwidth::radian)),
currency_(maxNeg(bitwidth::currency)),
count_(maxNeg(bitwidth::count)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we should exclude this here:

static constexpr int32_t min = -(static_cast<int32_t>(1U << Bits) / 2);
for the overflow checks, since it may be an error if all bases happen to be at their minimum? Or is it valid to have one or a few bases at minimum, given that it is extremely unlikely that all of them are?

This was referenced Mar 10, 2021
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.

3 participants