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

Fixed FTP download and upload file sizes being limited by available RAM ... #590

Merged
merged 1 commit into from Jul 7, 2014

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented May 12, 2014

...(#565).

@@ -229,7 +229,7 @@ public :
/// \param data Data containing the raw listing
///
////////////////////////////////////////////////////////////
ListingResponse(const Response& response, const std::vector<char>& data);
ListingResponse(const Response& response, const std::string& data);

This comment has been minimized.

@mantognini

mantognini May 12, 2014

Member

This changes the public API; does it have bad consequences? I guess it's only used by SFML internally but...

@mantognini

mantognini May 12, 2014

Member

This changes the public API; does it have bad consequences? I guess it's only used by SFML internally but...

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 12, 2014

Member

It is indeed only used internally. No problem to modify it.

Member

LaurentGomila commented May 12, 2014

It is indeed only used internally. No problem to modify it.

path += "/";
// Create the file and truncate it if necessary
std::ofstream file((path + filename).c_str(), std::ios_base::binary | std::ios_base::trunc);

This comment has been minimized.

@mantognini

mantognini May 12, 2014

Member

This could override a file. The change is consistent with the previous implementation (where data was only appended to the file) so it's okay for me. But the documentation doesn't say anything about that. This could be improved I think.

@mantognini

mantognini May 12, 2014

Member

This could override a file. The change is consistent with the previous implementation (where data was only appended to the file) so it's okay for me. But the documentation doesn't say anything about that. This could be improved I think.

This comment has been minimized.

@TankOs

TankOs Jun 11, 2014

Member

A separate ticket seems to be a good place for this IMHO.

@TankOs

TankOs Jun 11, 2014

Member

A separate ticket seems to be a good place for this IMHO.

char buffer[1024];
std::size_t count;
for (;;)

This comment has been minimized.

@mantognini

mantognini May 12, 2014

Member

I don't think we have discussed about infinite loop in our style guide. This is my personal opinion so let me know what you think.

I would rather use while (sending) and replace the two break statements with sending = false. Generally speaking I would avoid while(true) or similar pattern. It's more readable this way; no need to read the whole loop body to understand what's going one with the branching and breaking.

@mantognini

mantognini May 12, 2014

Member

I don't think we have discussed about infinite loop in our style guide. This is my personal opinion so let me know what you think.

I would rather use while (sending) and replace the two break statements with sending = false. Generally speaking I would avoid while(true) or similar pattern. It's more readable this way; no need to read the whole loop body to understand what's going one with the branching and breaking.

This comment has been minimized.

@Bromeon

Bromeon May 12, 2014

Member

We recently had a short discussion about loop styles and came to the conclusion that the current code is the cleanest solution. Has the original discussion been lost?

My opinion is: both code duplication and complicated code paths are bad. If an infinite loop solves either problem, I'm generally for it. I'm not a fan of artificial control variables, a break or return is usually simpler and attracts the reader's eye directly.

@Bromeon

Bromeon May 12, 2014

Member

We recently had a short discussion about loop styles and came to the conclusion that the current code is the cleanest solution. Has the original discussion been lost?

My opinion is: both code duplication and complicated code paths are bad. If an infinite loop solves either problem, I'm generally for it. I'm not a fan of artificial control variables, a break or return is usually simpler and attracts the reader's eye directly.

This comment has been minimized.

@mantognini

mantognini May 12, 2014

Member

It seems I've missed this conversation. :/

Well, I've a slightly different opinion on the matter, so I'd love here others' opinion. I would use condition variable unless it implies a complex loop body with lot of branching; in that case I would use break/continue/return.

@mantognini

mantognini May 12, 2014

Member

It seems I've missed this conversation. :/

Well, I've a slightly different opinion on the matter, so I'd love here others' opinion. I would use condition variable unless it implies a complex loop body with lot of branching; in that case I would use break/continue/return.

This comment has been minimized.

@TankOs

TankOs Jun 11, 2014

Member

I'd also use condition variables. @Bromeon mentioned complicated code paths, and return and friends do exactly break those paths. I rather like to have one starting and one ending point.

@TankOs

TankOs Jun 11, 2014

Member

I'd also use condition variables. @Bromeon mentioned complicated code paths, and return and friends do exactly break those paths. I rather like to have one starting and one ending point.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 12, 2014

Member

@LaurentGomila this seems reasonable, yes.

Member

mantognini commented May 12, 2014

@LaurentGomila this seems reasonable, yes.

Show outdated Hide outdated src/SFML/Network/Ftp.cpp Outdated
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 12, 2014

Member

I think I'm done with comments for now. :P

Member

mantognini commented May 12, 2014

I think I'm done with comments for now. :P

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone May 13, 2014

@eXpl0it3r eXpl0it3r added bug labels May 13, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.x, 2.2 May 13, 2014

@Bromeon Bromeon added the s:accepted label Jun 6, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Except the error reporting everybody seems to agree to this. Merge ok?

Member

TankOs commented Jun 11, 2014

Except the error reporting everybody seems to agree to this. Merge ok?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 4, 2014

Member

I fixed the outstanding issues mentioned in the previous diff. Everything should be OK in 362a590.

Member

binary1248 commented Jul 4, 2014

I fixed the outstanding issues mentioned in the previous diff. Everything should be OK in 362a590.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 5, 2014

Member

I've added this to my "merge list", if there are no further comments or issues raised, it will soon be merged.

Member

eXpl0it3r commented Jul 5, 2014

I've added this to my "merge list", if there are no further comments or issues raised, it will soon be merged.

@eXpl0it3r eXpl0it3r modified the milestones: 2.2, 2.x Jul 7, 2014

@eXpl0it3r eXpl0it3r merged commit 362a590 into master Jul 7, 2014

@eXpl0it3r eXpl0it3r deleted the bugfix/ftp_transfer branch Jul 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment