It looks like the build failed. (I didn't think to run a make all before committing, because it didn't seem like these changes would have any effect on other parts of the code.)

I think this might be because the C sin(), cos(), etc. defined in math.h can't be overloaded? Several places in the OkapiLib code use these functions instead of std::sin() etc. defined in <cmath>.

I'm honestly not sure how best to proceed from here. I could put the functions into their own namespace, or make them member functions of RQuantity, but that doesn't seem ideal.

Thanks for the feedback. To clarify, if (for instance) sin() returns only a unitless number, should it accept only an angle? Also, if the trig functions only work with angles and numbers, would it make more sense to put them in QAngle.hpp, or to have all the functions together in RQuantity.hpp?

It looks like there are more files that use (e.g.) sin() instead of std::sin() that are preventing the build from passing. I'm happy to go through and fix these if you want, but I don't really have a way to test the result.

To clarify, if (for instance) sin() returns only a unitless number, should it accept only an angle?

Yes.

Also, if the trig functions only work with angles and numbers, would it make more sense to put them in QAngle.hpp, or to have all the functions together in RQuantity.hpp?

Leave them in RQuantity.hpp.

I'm happy to go through and fix these if you want, but I don't really have a way to test the result.

Go ahead and change them.

Pow would probably look different than the other methods, as the exponent would probably have to be given as a template parameter for it to know the return type. A good compromise would be a square and cube method.

I agree. @Breq16 can you try to implement pow this way?

Rounding methods like floor and ceil are also useful, but I can't think of a good way to express them in terms of units

I agree they are useful and I don't think it would be difficult to implement. Wouldn't round, floor, and ceil simply return the same dimensions as they accepted?

I agree they are useful and I don't think it would be difficult to implement. Wouldn't round, floor, and ceil simply return the same dimensions as they accepted?

The problem is that the rounding depends on the internal unit - round would work differently with degrees than radians for example, so we would need to specify what unit we want to round to.

The problem is that the rounding depends on the internal unit - round would work differently with degrees than radians for example, so we would need to specify what unit we want to round to.

Oh, right. I forgot about that. I think it makes sense that we'd require the user to supply a specific unit to round in. For example, floor<inch>(1.2_in) == 1_in, round<meter>(47_in) == 1_m.

Finally, I think it would be a good idea to write some simple tests for the functionality of these new methods. Even if the highest chance of something breaking is within compile-time, there is always a possibility of having issues with certain types, or silly things like swapped parameters. Testing is definitely necessary for the rounding methods if you choose to implement them.

Writing tests are fairly straightforward with googletest, but if you aren't comfortable with the testing system I'm happy to write them.

I had a working implementation of pow() that used a std::ratio as a template parameter (so cubing a number would be e.g. pow<std::ratio<3>>(3_in)). The syntax isn't ideal (which is why I didn't include it in the initial PR), but I can't think of a better way to implement it. I'd be happy to add this as well as some more user-friendly square() and cube() functions.

Correct me if I'm wrong here, but it seems like the syntax for the rounding would be floor(1.2_in, inch), not floor<inch>(1.2_in), because inch is an instance of QLength, not a type? If so, implementing the rounding functions this way shouldn't be a problem.

It seems like the other log and exponent functions wouldn't really work because of the way units are handled (for instance, log(1_m) and log(1_ft) can't both return zero).

I don't really have any experience writing tests, but I could give it a try. (It seems like the correct place to add them is in test/unitTests.cpp?) I'll read the googletest documentation and try to write some.

For pow, you should be able to get away with accepting an int as the template parameter (it's one of the few things that templates allow), and doing pow<3>(3_in).

Humm, you are correct. I'm not actually sure how you would go about this then - the unit parameter would be an RQuantity of the same dimension as the value parameter, but then the only thing that you can use to tell the units apart is the ratio to their underlying unit (inch.getValue() == 0.0254). Maybe it would work if you divided the value by the unit ratio, used the rounding function, and then multiplied the value by the ratio.

Yep, that's where you should put the unit tests. You just make a test case, and the testing framework will automagically add it to the tests which the CI will run. You can also run the tests locally by running the cmake commands outlined by the azure build file (in the project root).

Cool, I'll get to work on the unit tests.

With regards to pow, it seems like it'd be best to allow users to raise an RQuantity to a fractional exponent as well (such as pow<std::ratio<1, 4>> for a fourth root). It seems like it's possible to have both this version and the nicer int version. Is it worth keeping both, or just the int version?

Yeah, I'd keep them both as overloads. It might also be nice to have a root<int> method to compliment the pow<int> method, and then the pow<R> is to do both.