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

fix for #307 #308

Merged
merged 8 commits into from
Aug 29, 2023
Merged

fix for #307 #308

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9d0062a
change the order of to_string processing for meters and seconds to ma…
phlptp Aug 28, 2023
dc45761
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 28, 2023
97ac486
code comments and switch second order
phlptp Aug 28, 2023
40024e5
Merge branch 'meters_second_fix' of https://github.com/LLNL/units int…
phlptp Aug 28, 2023
2f898c8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 28, 2023
b20621e
fix suppressions
phlptp Aug 28, 2023
b20477b
add a few more tests
phlptp Aug 29, 2023
cceacf9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 29, 2023
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
6 changes: 3 additions & 3 deletions config/cppcheck_suppressions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
unusedFunction:units/x12_conv.cpp:1024
unusedFunction:units/r20_conv.cpp:2738
unusedFunction:units/x12_conv.cpp:1009
passedByValue:units/units.cpp:222
passedByValue:units/units.cpp:1154
passedByValue:units/units.cpp:1171
passedByValue:units/units.cpp:224
passedByValue:units/units.cpp:1156
passedByValue:units/units.cpp:1173
9 changes: 9 additions & 0 deletions test/test_unit_strings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,15 @@ TEST(unitStrings, watthours)
EXPECT_EQ(str, "GWh*m");
}

TEST(unitStrings, mm)
{
auto speedUnit = unit_from_string("mm/s");
EXPECT_EQ(to_string(speedUnit), "mm/s");

auto accUnit = unit_from_string("mm/s^2");
EXPECT_EQ(to_string(accUnit), "mm/s^2");
Comment on lines +447 to +451
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked if these are the only examples, or is this part of a wider class of regressions that was introduced recently?

For example, km/s currently also formats as kHz*m, is this also fixed by the change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The cause of this issue is a combination of two things. The first being a check taking a single base unit away from the unit, and 'm' was first in the list. so the remaining portions of the unit were then converted in isolation and since 1/s=Hz it also adds any prefix, by swapping the order to dealing with 's' first it takes care combinations of 'm' and 's' a bit better. So as best as I can tell the only types of unit this issue is affecting is combinations of 'meter' and 'second' so yes it also fixes fixes km/s. and gray being m^2/s^2 is a bit of quirk as well, that comes from handling meters in this fashion before s^2 so adding that check first solves that issue as well.

}

TEST(unitStrings, customUnits)
{
EXPECT_EQ(to_string(precise::generate_custom_unit(762)), "CXUN[762]");
Expand Down
12 changes: 7 additions & 5 deletions units/units.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,11 @@ static const umap base_unit_names = getDefinedBaseUnitNames();

using ustr = std::pair<precise_unit, const char*>;
// units to divide into tests to explore common multiplier units
static UNITS_CPP14_CONSTEXPR_OBJECT std::array<ustr, 29> testUnits{
{ustr{precise::m, "m"},
ustr{precise::s, "s"},
static UNITS_CPP14_CONSTEXPR_OBJECT std::array<ustr, 30> testUnits{
{ustr{precise::s, "s"},
ustr{precise::s.pow(2), "s^2"}, // second squared need to come before
// meter to deal with accelleration units
ustr{precise::m, "m"},
ustr{precise::kg, "kg"},
ustr{precise::mol, "mol"},
ustr{precise::currency, "$"},
Expand Down Expand Up @@ -198,8 +200,8 @@ static UNITS_CPP14_CONSTEXPR_OBJECT std::array<ustr, 29> testUnits{
// units to divide into tests to explore common multiplier units which can be
// multiplied by power
static UNITS_CPP14_CONSTEXPR_OBJECT std::array<ustr, 6> testPowerUnits{
{ustr{precise::m, "m"},
ustr{precise::s, "s"},
{ustr{precise::s, "s"},
ustr{precise::m, "m"},
ustr{precise::radian, "rad"},
ustr{precise::km, "km"},
ustr{precise::ft, "ft"},
Expand Down