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

[REG2.065a] Issue 12098 - libcurl bad argument on handle null #1920

Merged
merged 1 commit into from
Feb 11, 2014

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Feb 10, 2014

https://d.puremagic.com/issues/show_bug.cgi?id=12098

Don't mix opCall and regular constructor for Type(...) syntax!

@CyberShadow
Copy link
Member

If this change silently broke Phobos, it may silently break user code. Posted my concerns here:
dlang/dmd#3221 (comment)

@monarchdodra
Copy link
Collaborator

If this change silently broke Phobos, it may silently break user code.

I have the same concern.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 10, 2014

@CyberShadow @monarchdodra To prevent silent braking, I opened dlang/dmd#3240. From its auto-tester result, you can see the effect of the diagnostic enhancement.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 10, 2014

So, how about this change?

@WalterBright
Copy link
Member

This series of cascading changes makes me nervous. It suggests we are not thinking these things through, and rewriting rules for static opCall at the last minute seems terribly dangerous.

Constructor taking the url as parameter.
*/
this(const(char)[] url)
static HTTP opCall(const(char)[] url)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the comment disappearing here and in the other edits?

@WalterBright
Copy link
Member

Looking at the curl.d, it seems the problem is that curl is trying to have default construction that is not the simple .init property. Trying to fudge how the language is supposed to work is going to cause problems, and we're seeing that effect. It's like stepping on a bulge in a rug - it just shifts the bulge to another place.

@ghost
Copy link

ghost commented Feb 10, 2014

Looking at the curl.d, it seems the problem is that curl is trying to have default construction that is not the simple .init property.

Which is a problem people run into all the time, being desperate for a default ctor. If people are trying all means necessary to be able to define default construction then maybe the language should start supporting that.

I really don't think people are going to get used to the missing default ctor (it's been like that for years..), they're just going to keep asking for it.

@CyberShadow
Copy link
Member

I've yet to hear a solid explanation for why D doesn't have default construction. All I've heard so far was vague hand-waving about "generic code and .init", but no actual explanation why and when this is a good thing, and how it outweighs the benefits of default construction.

@monarchdodra
Copy link
Collaborator

I've yet to hear a solid explanation for why D doesn't have default construction.

It's about having all Types having a statically known initial state. I seem to remember that it helps for "initializing" statics.

Looking at the curl.d, it seems the problem is that curl is trying to have default construction that is not the simple .init property. Trying to fudge how the language is supposed to work is going to cause problems, and we're seeing that effect. It's like stepping on a bulge in a rug - it just shifts the bulge to another place.

Please keep in mind that it may not be "default" construction, but simply "explicit runtime-initialization with no specified argument", which is also something D doesn't support. And I think there is no good argument to not support that.

The "static opCall trick" is the "recommended" way to emulate that. We should be very careful about changing its behavior. Or if not, provide a real way to say: "Please runtime initialize. Arguments: None."

@JakobOvrum
Copy link
Member

The "static opCall trick" is the "recommended" way to emulate that. We should be very careful about changing its behavior. Or if not, provide a real way to say: "Please runtime initialize. Arguments: None."

I think it's frivolous syntax sugar over what it really is: a static constructor method (although a free function works nicely too). opCall conflicting with uniform construction is probably good evidence that a named constructor method is the way to go (optionally paired with @disable this();).

edit:

That is to say, I think it's just a deceptive case of operator overload abuse.

@monarchdodra
Copy link
Collaborator

I think it's frivolous syntax sugar over what it really is: a static constructor method (although a free function works nicely too).

Maybe, but there is still little support for it. The issue with "named" constructors is that they aren't constructors: they are just functions that return a new instance. It wouldn't work with, say, structs with disabled postblits (because it may or may not postblit). It would also create problems for "emplace", which only knows about constructors, not "constructor-like functions".

EDIT: As a matter of fact, I "recently" changed emplace to explicitly NOT use opCall, instead suggesting to use emplace(chunk, T());, leaving the responsibility of the call to opCall to the caller.

A "simple" workaround would be to use a auto a = this(noArg), which would call this(NoArg). However, it would be nice to have the library provide the type/instance, so each class doesn't re-invent its own tag. Furthermore, this would work nicelly with emplace too. Having the language/library "herd" us to a specific usage, rather than just saying "Nope, can't be done. Your design is wrong."

@JakobOvrum
Copy link
Member

Maybe, but there is still little support for it. The issue with "named" constructors is that they aren't constructors: they are just functions that return a new instance. It wouldn't work with, say, structs with disabled postblits (because it may or may not postblit). It would also create problems for "emplace", which only knows about constructors, not "constructor-like functions".

I'm pretty sure any function that returns a struct by value and has only one return statement (or several that return the same named local variable) are guaranteed to move, which does not call postblit.

As you pointed out, the latter problem is shared with opCall.

However, it would be nice to have the library provide the type/instance, so each class doesn't re-invent its own tag. Furthermore, this would work nicelly with emplace too. Having the language/library "herd" us to a specific usage, rather than just saying "Nope, can't be done. Your design is wrong."

Yeah, a standard way to nullary-construct a struct sounds beneficial.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 11, 2014

Looking at the curl.d, it seems the problem is that curl is trying to have default construction that is not the simple .init property. Trying to fudge how the language is supposed to work is going to cause problems, and we're seeing that effect.

That's right. Therefore so, this is legitimate change to me.

If people are trying all means necessary to be able to define default construction then maybe the language should start supporting that.

Today, by relying on the full CTFE feature, it might be possible.
But, currently D is designed that default construction is always cheap.

Please keep in mind that it may not be "default" construction, but simply "explicit runtime-initialization with no specified argument", which is also something D doesn't support. And I think there is no good argument to not support that.

I think supporting it will introduce more confusion for both C++ experienced programmers (With T t; and T t = T();, the former will not call the added "default constructor"), and experienced D programmers (T t; and T t = T(); will have different behavior.)

The "static opCall trick" is the "recommended" way to emulate that.

But first, we should not recommend to use static opCall() for the beginners. In D, "elaborate default construction" is not supported. so if a programmer wants to emulate it, his code should become ugly.

I'm pretty sure any function that returns a struct by value and has only one return statement (or several that return the same named local variable) are guaranteed to move, which does not call postblit.

I'm continuously working to add NRVO to language specification. It's important feature in semantic level.

@WalterBright
Copy link
Member

I'll pull this after the comments are fixed, even though I think the code is fundamentally broken in providing for non-trivial default construction. The reason is, of course, for backwards compatibility.

I think supporting it will introduce more confusion for both C++ experienced programmers (With T t; and T t = T();, the former will not call the added "default constructor"), and experienced D programmers (T t; and T t = T(); will have different behavior.)

I agree. All those forms (including T t = T.init;) should all produce exactly the same result. Anything different will be disastrously confusing.

being desperate for a default ctor.

The problem with a default ctor is then what does .init mean? Things start coming unglued at the seams, and this bug and pull request are prime examples.

Having a statically known .init value has a number of wonderful properties, such as no dependency ordering issues with construction, construction that cannot fail, guaranteed O(1) behavior, a known null-object pattern, the non-trivial constructors always start with a guaranteed known state, etc.

There's always a price to pay, but I strongly disagree that it is a 'desperate' issue. It's at worst a minor issue, easily solved with an initialize or factory function.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 11, 2014

I'll pull this after the comments are fixed,

OK, I reverted comment removing.

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 11, 2014

All green.

WalterBright added a commit that referenced this pull request Feb 11, 2014
[REG2.065a] Issue 12098 - libcurl bad argument on handle null
@WalterBright WalterBright merged commit 66b3b88 into dlang:master Feb 11, 2014
@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 11, 2014

Thanks.

9rnsr pushed a commit to 9rnsr/phobos that referenced this pull request Feb 11, 2014
[REG2.065a] Issue 12098 - libcurl bad argument on handle null
@9rnsr 9rnsr deleted the fix12098 branch February 11, 2014 06:25
@ghost
Copy link

ghost commented Feb 11, 2014

Having a statically known .init value has a number of wonderful properties, such as no dependency ordering issues with construction

I'm not sure what this means.

construction that cannot fail

And why is this important? If construction fails, it fails.

guaranteed O(1) behavior

__traits(hasCustomDefaultCtor, T). Then templates can statically disallow accepting such objects if they require their default construction to be O(1).

a known null-object pattern

Known at compile-time? I don't see what this buys us.

the non-trivial constructors always start with a guaranteed known state

Again I don't see what this buys us.

You keep listing things that .init provides, but you never mention in what context this is ever necessary to have. What code would break or be impossible to rewrite that depends on not being able to define a custom default ctor?

@monarchdodra
Copy link
Collaborator

It's at worst a minor issue

Actually, it is a very regular source of bugs and issues for any value type with reference semantics. Objects are passed "non-initialized", then lazilly initialized in the called function, but the caller never sees the change. Learn is riddled with the question, and they are regularly filed in the bug reports.

For example: AA's. The damn things still can't be initialized to a state that references an empty payload. You claim "confusion between T.init and T()". I guess appender!T and Appender!T is so much less ambiguous :/

Also, have you looked at what the code for things such as Appender or Array? Each and every function has to lazilly initializes. It's a performance and maintainability problem.

Every time anybody tries to write a struct with a pointer to a payload, he faces the issue of "How am I going to initialize the damn thing? Am I really going to have to lazily instantiate in every function?" It's an absolute nightmare.

easily solved with an initialize or factory function.

Maybe, but I feel it's our responsibility to promote an easy and reusable solution. Hand waving it away as "trivial" and acting like it doesn't exist won't make the problem go away. While I'm not requesting we have default constructors, the language does a very poor job at supporting types that do need initialization. And I know for a fact it's on a lot of people's "List of things I don't like in D".

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.

5 participants