nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
962 stars 138 forks source link

Math functions and ADL #39

Open Morwenn opened 7 years ago

Morwenn commented 7 years ago

Why did you decide to put the math functions in a math:: subnamespace?

I really think that compile-time unit types are extremely useful tools to have at hand (standardization would be great), but people are more likely to migrate their code if changing their unitless types by unit types works out-of-the-box. I often develop generic algorithms, including generic mathematical algorithms where math functions are found via ADL in the implementation:

using std::abs;
auto res = abs(value);

Such algorithms could work out-of-the-box with suitable units types (when the operations make sense) had the mathematical functions been in the same namespace as the unit types, but your documentation says that such calls would be ambiguous.

Do you think that ADL-found mathematical functions for your unit types would be a solvable problem?

nholthaus commented 7 years ago

Honestly, I guess I just never use ADL because I pretty much exclusively use fully qualified namespaces in my code, and I didn't put thought into it. I think it would be a great addition.

I was worried about ambiguities during the original design, but I don't think there actually are many, if any (unless the committee later decides to template <cmath> or something).

Not being an ADL expert, is the fact that the units themselves are separated into namespaces (e.g. units::length, units::time) also going to prevent ADL from determining the correct math functions if they were defined in the units namespace?

Morwenn commented 7 years ago

Honestly I'm not sure how it works with nested namespaces. I guess that if all your unit classes inherit from a base class and you place your math functions in the base class namespace and make them take the base class, then it may work.

But I'd have to read more about ADL and check whether this is true. I'm totally unsure about whether it would actually work.

nholthaus commented 7 years ago

it shouldn't be too hard for me to prototype and find out. I'll give it a shot.

Morwenn commented 6 years ago

I remembered about this issue and thought about this a bit: when an unqualified call to a function is found, ADL will look up in the namespace of the argument to find a function with the appropriate name, but also in the namespaces of the base classes of the argument.

Basically this means that you could have a hidden namespace with a base_unit type and all the math functions taking base_unit in the same namespace. Every unit could then inherit from base_unit to benefit from ADL-found math functions. More realistically, it is clear that only a subset of math functions works for a given unit (hence the static assertions on unit categories of the current design), so you would need to have more fine-grained hidden unit bases for every unit categories so that ADL only works when a math function is applied to a unit that is supposed to work with it.

This solution would probably work well for the units defined in the library, but I'm not sure about user-defined ones; they probably would have to perform more work by themselves. That said, users already can't bypass the static assertions in the current math functions unless they make their units satisfy the traits (and those traits probably don't always cover their own unit categories), so the ADL solution would actually be more scalable despite requiring some work from user-defined unit implementers. I still think that making generic algorithms that find math functions via ADL work with unit is a problem worth solving :)

nholthaus commented 6 years ago

@Morwenn So I implemented this in my 3.0 dev branch and it's working really nicely, good call on your part. It's actually really making me want to improve the math section further, namely by supporting as many constexpr math functions as possible. Is your smath library in a good place to pull in as a dependency to enable that?

Right now I'm planning to give up on the single-header approach here since the build systems have matured a lot and it's still bogging down the compile time.

Morwenn commented 6 years ago

To be honest my static_math library was merely a toy project: I hardly know anything about numerical analysis, Taylor series and all those things, and most of the difficult parts have been contributed by other people; I wouldn't be able to fix bugs in those functions. For a quality implementations of constexpr math functions I would rather look at the math parts of Bolero Murakami's Sprout library (which also contains many more things, but the math part can probably be safely extracted).

I've looked at your code and it does indeed work with ADL nowadays, but mostly because ADL is a bit too greedy: it currently looks in the associated namespace of a class (and the associated namespaces of its base classes) for a function with the appropriate name. This is fine, but be careful: if P0934 makes it into a future standard revision, ADL will only look in a the associated namespace of a class (or base classes) for functions that explicitly name the class (or base class) and won't pick unconstrained templates in the associated namespaces. The proposal probably won't make it into the standard because it breaks stuff that works today (including your ADL-found math functions), but it also solves several problems due to ADL being too greedy :/