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.

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

Implemented as<T>() and is<T>() with accompanying tests #1075

Closed
wants to merge 3 commits into from

Conversation

JulianvDoorn
Copy link

Solves issue #1066

@dota17
Copy link
Member

dota17 commented Oct 29, 2019

Run ninja -v -C build-${LIB_TYPE} clang-format before submitting your code to format your code style, otherwise, it will fail in Travis-CI. And of course, you should run ninja -v -C build-${LIB_TYPE} to build your project to see if there is any build error after your changing. please see CONTRIBUTING.md

template <class T> struct isLookupHelper;

#define defIsLookupHelper(type, lookup) \
template <> struct isLookupHelper<type> { \
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid macros here. There are only a few types.
The "helper" structs are not needed.
They should have better names if they remain, and not be public.

We should be able to implement the as<T> and is<T> as member function template specializations directly, without forwarding to a helper struct.

defIsLookupHelper(UInt64, isUInt64);
defIsLookupHelper(double, isDouble);
defIsLookupHelper(const char*, isString);
defIsLookupHelper(String, isString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you've invented a new accessor: is<const char*>.
It does the same thing as is<String> but has a distinct name. I think that's confusing and we should remove the const char* specialization.

@cdunn2001 cdunn2001 mentioned this pull request Nov 4, 2019
@BillyDonahue
Copy link
Contributor

This was finished off and pushed as PR #1080 . Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants