Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Remove conversion.hpp dependency from public headers #12665

Merged
merged 2 commits into from
Aug 19, 2018
Merged

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Aug 17, 2018

Moves the template definition to a _impl.hpp header file in src.

@kkaefer kkaefer added refactor Core The cross-platform C++ core, aka mbgl labels Aug 17, 2018
@kkaefer kkaefer requested a review from jfirebaugh August 17, 2018 23:06
@jfirebaugh
Copy link
Contributor

Hmm, I don't think this is going to work. Convertible and ConversionTraits have to be accessible to SDKs so that they can implement conversions from Java Object and Objective-C id.

I'll revert #12414 while we figure out what to do to fix the build

@kkaefer kkaefer force-pushed the conversion-private branch from d345f16 to 1f9ecee Compare August 17, 2018 23:26
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Ok, I see how this is intended to work now. Thanks for fixing the build.

template optional<PropertyValue<TextTransformType>> Converter<PropertyValue<TextTransformType>>::operator()(conversion::Convertible const&, conversion::Error&, bool, bool) const;
template optional<PropertyValue<TranslateAnchorType>> Converter<PropertyValue<TranslateAnchorType>>::operator()(conversion::Convertible const&, conversion::Error&, bool, bool) const;

// template optional<PropertyValue<AlignmentType>> convert<PropertyValue<AlignmentType>, bool, bool>(const Convertible&, Error&, bool&&, bool&&);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

return PropertyExpression<T>(std::move(*expression), defaultValue);
}

template optional<PropertyExpression<AlignmentType>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty tempting to use a macro here:

#define EXPLICIT_INSTANTIATION(T) template optional<PropertyExpression<T>> \
    convertFunctionToExpression<T>(const Convertible&, Error&, bool);

EXPLICIT_INSTANTIATION(AlignmentType);
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I refrained from adding this because it's tricky to get complex typenames (e.g. std::array<float, 2>) to work correctly in a macro context. You could probably swing something with __VA_ARGS__, but that's nonstandard.

struct Error { std::string message; };

template <typename T>
class ConversionTraits;

class Convertible {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here explaining that this is a minimal header suitable for obtaining forward declarations, and that implementation files that need to actually perform conversions should include conversion_impl.hpp.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants