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

Removed greedy template definition of NanNew(...) #209

Closed
wants to merge 3 commits into from

Conversation

agnat
Copy link
Collaborator

@agnat agnat commented Oct 29, 2014

In #208 I proposed to remove the generic version of template<typename T, typename P> v8::Local<T> NanNew(P arg1) because it is error-prone.

This PR removes the function definition leaving only a declaration. It also adds the specializations required by the test suite.

Not sure I got everything right and to your liking, but I think it is enough to outline what needs doing.

HTH

@agnat
Copy link
Collaborator Author

agnat commented Oct 29, 2014

I should mention that this approach only triggers a compiler warning. The resulting module fails to load because of undefined symbols. However, proper compile time exceptions are a matter of taste and should be your decision.

I tested it on node v0.10.32 and v0.11.14 on Mac OS.

}

template <>
NAN_INLINE v8::Local<v8::External> NanNew<v8::External>(void * value) {

This comment was marked as off-topic.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 29, 2014

How can one raise a compile-time error when a function is used? #error does not seem limited to function scope.

@bnoordhuis
Copy link
Member

I should mention that this approach only triggers a compiler warning. The resulting module fails to load because of undefined symbols.

Everything has inline linkage, right? That means the compiler should either resolve everything at compile time or fail.

@agnat
Copy link
Collaborator Author

agnat commented Oct 29, 2014

How can one raise a compile-time error when a function is used? #error does not seem limited to function scope.

Hehe, TIMTOWTDI. Without vouching for their quality here are a few examples. And in C++11 there is static_assert(...).

Everything has inline linkage, right? That means the compiler should either resolve everything at compile time or fail.

AFAIK, inline linkage is more of a hint and the compiler is still free not to inline. It doesn't fail at link time because of the relaxed undefined symbol handling in node add-ons. The missing symbol might still be provided by some other compilation unit at dynamic load time.

BTW, I think the inline keyword is unnecessary, because template functions default to inline linkage.

@bnoordhuis
Copy link
Member

AFAIK, inline linkage is more of a hint and the compiler is still free not to inline.

That is correct but not what I mean. The inline keyword in C++ is like static in C: it means that the function is visible in the current compilation unit only and has no external linkage. That is why you should be getting compilation errors, not dynamic linker errors.

I wonder if NAN_INLINE evaluates to the wrong thing. That macro is pretty pointless, IMO. I suggest we s/NAN_INLINE/inline/g it.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 29, 2014

No C++ 11. Unfortunately we have to support compilers that do not even handle C++ 98 correctly.

something like this might work:

template <typename T>
void must_be_specialized(T const&)
{
   enum dummy { d = (sizeof(struct must_be_specialized_for_this_type)
                     == sizeof(T)) };
}

NAN_INLINE is a replica of V8_INLINE.

@bnoordhuis
Copy link
Member

That is why you should be getting compilation errors, not dynamic linker errors.

Guess I found the answer. Add-ons are compiled with -undefined dynamic_lookup on OS X.

@agnat
Copy link
Collaborator Author

agnat commented Oct 29, 2014

I suggest we s/NAN_INLINE/inline/g

+1 and s/NAN_INLINE//g in template functions. It's just noise, really.

Add-ons are compiled with -undefined dynamic_lookup on OS X.

That's what I meant by "relaxed undefined symbol handling in node add-ons".

something like this might work: [...]

Play with it! This is the fun part...

@agnat
Copy link
Collaborator Author

agnat commented Oct 29, 2014

Another thing is: Without this patch nan contains this little code factory at API level. We can't be sure that we don't break anything because we don't know how creative people are regarding typename P. How about small integers like int8_t, &c.? How about custom types with conversion operators?

Maybe this is a 2.0 thing after all. Just a thought, though...

@agnat
Copy link
Collaborator Author

agnat commented Oct 29, 2014

@kkoopa, if you're interested in the gritty details of C++ templates I'd recommend C++ Templates - The Complete Guide.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 29, 2014

Seems good as a reference, I'll bookmark it. I wrote the code in question and encountered all these issues at the time, that is why this function looks the way it does. After finishing I swore never to touch it again and hastily unlearned it all. Say what you want, but compared to e.g. Haskell, the template syntax of C++ is just awful, limitations and warts included.

On October 29, 2014 8:36:47 PM EET, David Siegel notifications@github.com wrote:

@kkoopa, if you're interested in the gritty details of C++ templates
I'd recommend C++ Templates - The Complete
Guide
.


Reply to this email directly or view it on GitHub:
#209 (comment)

@agnat
Copy link
Collaborator Author

agnat commented Nov 4, 2014

Superseded by PR above.

@agnat agnat closed this Nov 4, 2014
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