Skip to content

std.curl: document public methods #4314

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

Merged
DmitryOlshansky merged 1 commit into from
Jun 3, 2016
Merged

std.curl: document public methods #4314

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
39 changes: 21 additions & 18 deletions std/net/curl.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,6 @@ version(unittest)
}
version(StdDdoc) import std.stdio;

extern (C) void exit(int);

// Default data timeout for Protocols
private enum _defaultDataTimeout = dur!"minutes"(2);

Expand Down Expand Up @@ -1510,12 +1508,10 @@ private mixin template WorkerThreadProtocol(Unit, alias units)
}
}

// Workaround bug #2458
// It should really be defined inside the byLineAsync method.
// Do not create instances of this struct since it will be
// moved when the bug has been fixed.
// @@@@BUG 15831@@@@
// this should be inside byLineAsync
// Range that reads one line at a time asynchronously.
Copy link
Contributor

@JackStouffer JackStouffer May 12, 2016

Choose a reason for hiding this comment

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

Until this struct is moved into byLineAsync, this comment should stay

Copy link
Contributor Author

@wilzbach wilzbach May 12, 2016

Choose a reason for hiding this comment

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

the bug is fixed, but afaik having struct outsides is currently better because it avoids the long names. Ideally we would disallow access to it, but that would be yet another deprecation cycle?

Copy link
Contributor

@JackStouffer JackStouffer May 12, 2016

Choose a reason for hiding this comment

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

Ok, the comment can be removed and this can be moved inside when https://issues.dlang.org/show_bug.cgi?id=15831 is fixed.

Maybe add this note:

// @@@@BUG 15831@@@@
// this should be inside byLineAsync

Copy link
Contributor Author

@wilzbach wilzbach May 12, 2016

Choose a reason for hiding this comment

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

as far as i understood 15831 it just increases the binary size and people are already working against this problem, so we don't need to let us be limited by this. What do other people think?
btw as this symbol wasn't documented and had an explicit warning against calling it, i think it's okay to set it private or move in the function.

Copy link
Contributor

@JackStouffer JackStouffer May 25, 2016

Choose a reason for hiding this comment

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

as far as i understood 15831 it just increases the binary size

It's not just that. Your link times also increase exponentially with the symbol sizes. Also, debugging with symbols that are > 2000 characters sucks.

and people are already working against this problem

People are working on compression of symbol names, which is a bandaid and not solution. You still end up with slower compile times, exponential symbol size, and an extra step to debugging.

i think it's okay to set it private or move in the function.

Sure, set it to private, I'd argue that the above mentioned comment should be there as well.

static struct AsyncLineInputRange(Char)
private static struct AsyncLineInputRange(Char)
{
private Char[] line;
mixin WorkerThreadProtocol!(Char, line);
Expand All @@ -1539,7 +1535,6 @@ static struct AsyncLineInputRange(Char)
}
}


/** HTTP/FTP fetch content as a range of lines asynchronously.
*
* A range of lines is returned immediately and the request that fetches the
Expand Down Expand Up @@ -1664,13 +1659,10 @@ unittest
}
}


// Workaround bug #2458
// It should really be defined inside the byLineAsync method.
// Do not create instances of this struct since it will be
// moved when the bug has been fixed.
// @@@@BUG 15831@@@@
// this should be inside byLineAsync
// Range that reads one chunk at a time asynchronously.
static struct AsyncChunkInputRange
private static struct AsyncChunkInputRange
{
private ubyte[] chunk;
mixin WorkerThreadProtocol!(ubyte, chunk);
Copy link
Contributor

@JackStouffer JackStouffer May 12, 2016

Choose a reason for hiding this comment

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

ditto

Expand Down Expand Up @@ -2445,13 +2437,15 @@ struct HTTP
return http;
}

///
static HTTP opCall()
{
HTTP http;
http.initialize();
return http;
}

///
HTTP dup()
{
HTTP copy;
Expand Down Expand Up @@ -3183,13 +3177,15 @@ struct FTP
return ftp;
}

///
static FTP opCall()
{
FTP ftp;
ftp.initialize();
return ftp;
}

///
FTP dup()
{
FTP copy = FTP();
Expand Down Expand Up @@ -3524,6 +3520,7 @@ struct SMTP
return smtp;
}

///
static SMTP opCall()
{
SMTP smtp;
Expand Down Expand Up @@ -3973,7 +3970,7 @@ struct Curl
{
alias OutData = void[];
alias InData = ubyte[];
bool stopped;
private bool _stopped;

private static auto ref curl() @property { return CurlAPI.instance; }

Copy link
Contributor

@JackStouffer JackStouffer May 12, 2016

Choose a reason for hiding this comment

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

See comment on previous PR

Copy link
Contributor Author

@wilzbach wilzbach May 12, 2016

Choose a reason for hiding this comment

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

access is provided, see below. Though we might want to deprecate that.
Setting access should trigger an error as you shouldn't be able to set the internal state.

Copy link
Contributor

@JackStouffer JackStouffer May 12, 2016

Choose a reason for hiding this comment

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

No need, I didn't see the function. Just mark it @property and it should be good.

Copy link
Contributor Author

@wilzbach wilzbach May 25, 2016

Choose a reason for hiding this comment

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

It's already @property ;-)

Expand All @@ -4000,10 +3997,16 @@ struct Curl
enforce!CurlException(!handle, "Curl instance already initialized");
handle = curl.easy_init();
enforce!CurlException(handle, "Curl instance couldn't be initialized");
stopped = false;
_stopped = false;
set(CurlOption.nosignal, 1);
}

///
@property bool stopped() const
{
return _stopped;
}

/**
Duplicate this handle.

Expand All @@ -4016,7 +4019,7 @@ struct Curl
{
Curl copy;
copy.handle = curl.easy_duphandle(handle);
copy.stopped = false;
copy._stopped = false;

with (CurlOption) {
auto tt = AliasSeq!(file, writefunction, writeheader,
Expand Down Expand Up @@ -4094,7 +4097,7 @@ struct Curl
void shutdown()
{
throwOnStopped();
stopped = true;
_stopped = true;
curl.easy_cleanup(this.handle);
this.handle = null;
}
Expand Down