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

Avoid colliding with macro names B1 and PI #247

Merged
merged 1 commit into from
Jun 22, 2024
Merged

Avoid colliding with macro names B1 and PI #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions au/magnitude.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,16 @@ using CommonMagnitudeT = typename CommonMagnitude<Ms...>::type;
// Value based interface for Magnitude.

static constexpr auto ONE = Magnitude<>{};

#ifndef PI
// Some users must work with frameworks that define `PI` as a macro. Having a macro with this
// easily collidable name is exceedingly unwise. Nevertheless, that's not the users' fault, so we
// 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>{};
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😓

  1. Use Google style guide for naming constants kBlah. It generally won't collide with macros, but it is ugly.
  2. Rename PI to something like AUPI. It would be unique, but it's more verbose and farther from the simple Greek letter. Also, that is what namespaces are for 😄
  3. Define PI in another header. This would require more fine grained header management.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

#endif

template <typename... BP1s, typename... BP2s>
constexpr auto operator*(Magnitude<BP1s...>, Magnitude<BP2s...>) {
Expand Down
16 changes: 8 additions & 8 deletions au/stdx/type_traits.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,18 @@ using bool_constant = std::integral_constant<bool, B>;
// Source: adapted from (https://en.cppreference.com/w/cpp/types/conjunction).
template <class...>
struct conjunction : std::true_type {};
template <class B1>
struct conjunction<B1> : B1 {};
template <class B1, class... Bn>
struct conjunction<B1, Bn...> : std::conditional_t<bool(B1::value), conjunction<Bn...>, B1> {};
template <class B>
struct conjunction<B> : B {};
template <class B, class... Bn>
struct conjunction<B, Bn...> : std::conditional_t<bool(B::value), conjunction<Bn...>, B> {};

// Source: adapted from (https://en.cppreference.com/w/cpp/types/disjunction).
template <class...>
struct disjunction : std::false_type {};
template <class B1>
struct disjunction<B1> : B1 {};
template <class B1, class... Bn>
struct disjunction<B1, Bn...> : std::conditional_t<bool(B1::value), B1, disjunction<Bn...>> {};
template <class B>
struct disjunction<B> : B {};
template <class B, class... Bn>
struct disjunction<B, Bn...> : std::conditional_t<bool(B::value), B, disjunction<Bn...>> {};

// Source: adapted from (https://en.cppreference.com/w/cpp/types/negation).
template <class B>
Expand Down
2 changes: 1 addition & 1 deletion au/units/degrees.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ struct DegreesLabel {
};
template <typename T>
constexpr const char DegreesLabel<T>::label[];
struct Degrees : decltype(Radians{} * PI / mag<180>()), DegreesLabel<void> {
struct Degrees : decltype(Radians{} * Magnitude<Pi>{} / mag<180>()), DegreesLabel<void> {
using DegreesLabel<void>::label;
};
constexpr auto degree = SingularNameFor<Degrees>{};
Expand Down
Loading