Avoid colliding with macro names B1 and PI
#247
Conversation
This helps Arduino compatibility.
Of course, this whole situation is an excellent case in point to
illustrate why, if you _must_ use macros, _never_ to give them such
easily collidable names. But that is not the users' fault, so since we
can work around the problem, we do so.
The `B1` template parameter can be renamed to `B` without loss of
readability.
`PI` is a little more challenging. We do use it in one place in our
code, in defining `Degrees`, so we must replace that with its definition
of `Magnitude<Pi>{}`. We provide workaround instructions in the
comments. We leave alone the many instances of `PI` in test files,
since that shouldn't affect these users.
chiphogg
added
the
release notes: 🐛 lib (bugfix)
| // accommodate those frameworks by omitting the definition of `PI` in this case. | ||
| // | ||
| // If you are stuck with such a framework, you can choose a different name that does not collide, | ||
| // and reproduce the following line in your own system. | ||
| static constexpr auto PI = Magnitude<Pi>{}; |
There was a problem hiding this comment.
I'm not sure if this is a perfect solution. It would depend on the order of includes. The order of headers is not great to reason about.
I don't have great ideas for alternative solutions 😓
- Use Google style guide for naming constants
kBlah. It generally won't collide with macros, but it is ugly. - Rename
PIto something likeAUPI. It would be unique, but it's more verbose and farther from the simple Greek letter. Also, that is what namespaces are for 😄 - Define PI in another header. This would require more fine grained header management.
There was a problem hiding this comment.
Having slept on this, I'm leaning towards Option 4: just don't define PI. Using really short all-caps names is a recipe for collision with poorly designed frameworks. That's not to say we'll never do it, but it seems wise to minimize how much we do it. And in this case, we have a clear collision with the Arduino framework, which we'd really like to support, so I think our PI has to go.
I'll deprecate it in a follow on PR for ease of review. I think we'll still want to retain the #ifndef guards until we remove it entirely (probably in 0.4.0).
There was a problem hiding this comment.
I'm leaning towards 1. It adds one ugly character, but it is consistent and reduces conflicts.
Deleting it for now buys us time. It's a good idea to minimize Au's public API. At some point, we should design a path forward for constants that works for most users.
It conflicts with the `pascal` macro in `<Windows.h>`. Yes, they really did define a lowercase-named macro in `<Windows.h>`. As with `PI` (#247), the `#ifndef` should immediately unblock most users, but it's not a long-term solution, because it depends on order-of-includes. Therefore, we also deprecate `pascal` (as with `PI` in #250). I think a good rule of thumb is that deprecated constructs should be deprecated for at least one full minor release cycle, so we'll plan to delete it (along with `PI`) as part of 0.5.0. Fixes #284.
It conflicts with the `pascal` macro in `<Windows.h>`. Yes, they really did define a lowercase-named macro in `<Windows.h>`. As with `PI` (#247), the `#ifndef` should immediately unblock most users, but it's not a long-term solution, because it depends on order-of-includes. Therefore, we also deprecate `pascal` (as with `PI` in #250). I think a good rule of thumb is that deprecated constructs should be deprecated for at least one full minor release cycle, so we'll plan to delete it (along with `PI`) as part of 0.5.0. Fixes #284.
This helps Arduino compatibility.
Of course, this whole situation is an excellent case in point to
illustrate why, if you must use macros, never to give them such
easily collidable names. But that is not the users' fault, so since we
can work around the problem, we do so.
The
B1template parameter can be renamed toBwithout loss ofreadability.
PIis a little more challenging. We do use it in one place in ourcode, in defining
Degrees, so we must replace that with its definitionof
Magnitude<Pi>{}. We provide workaround instructions in thecomments. We leave alone the many instances of
PIin test files,since that shouldn't affect these users.
Helps #246.