Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert "std: Standardize (input, output) param orderings" #24142

Closed
wants to merge 1 commit into from

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Apr 7, 2015

I don't know if I agree that this needed an RFC, but this was definitely the wrong call IMHO. I think it's bad enough that it should be a beta-period breaking change.

This reverts commit acd48a2.

r? @aturon

@emberian
Copy link
Member

emberian commented Apr 7, 2015

I agree completely, and think this warrants releasing a beta update.

@emberian
Copy link
Member

emberian commented Apr 7, 2015

Though, eternaleye brings up in IRC:

However, I do think "consistency with rust" is more critical than 'consistency with C's memcpy'
I mean, if the goal is for Rust to take over - then "consistency with C" is a time-limited benefit
Inconsistency with the rest of Rust is pain forever.

@pythonesque
Copy link
Contributor

Agreed with the first two comments.

@pcwalton
Copy link
Contributor Author

pcwalton commented Apr 7, 2015

Sure, but I think we should make it consistent the other way: destination, then source.

@eternaleye
Copy link
Contributor

@pcwalton
I'm of two minds about that.

On the one hand, there are a number of places we already have destination-like stuff on the left, the biggest being 'let' and assignment in general, followed by clone_from.

On the other hand, Rust code is read left-to-right; inside of a function's arguments, it feels natural to me that data will flow in the same direction as the reader's attention.

Similarly, when putting the returned data in the return type is infeasible, the end of the argument list is the "nearest" position visually.

@CloudiDust
Copy link

@eternaleye, I see copy in I/O to be like the cp command and flows left to right, while copy in memory management to be C-like and flows right to left.

So IMHO the original behaviour before the slient breaking change is fine. However this is (unfortunately) a result of inconsistent precedents. If we can truly start anew, we should consistently do src, dst or dst, src, but currently I prefer the original behaviour.

@CloudiDust
Copy link

That said, such inconsistency is still a problem and I can see why people would prefer breaking from the C convention.

So maybe we can use a "painfully clear" name that indicates the direction, like memcopy_from_to?

@petrochenkov
Copy link
Contributor

So, after one silent breakage people fixed their code in accordance with the new parameter ordering and now you are going to break it again.

@CloudiDust
Copy link

I'll publish an RFC about renaming the functions, along the line of memory_copy_ft to prevent people from assuming the C convention in the future.

Currently I am of the opinion that reverting may actually add to the confusion.

@adrientetar
Copy link
Contributor

Now that it's done, I say we should keep it and deal with it. It the long run this ordering makes more sense and only a new language can afford to commit such changes.

@vladimir-lu-work
Copy link

-1 to merging this pull request. For what it's worth I think it is better to stay consistent within Rust, and if that means a break for applications because of reversed argument order then so be it. It would be very helpful to have a lint that warns about mut -> const conversion.

@Tobba
Copy link
Contributor

Tobba commented Apr 7, 2015

-1
The damage has already been done, let's not make it worse. The new orderings are more consistent with the rest of Rust anyways.

@CloudiDust
Copy link

On second thought, the five affected functions:

ptr::copy
ptr::copy_nonoverlapping
slice::bytes::copy_memory
intrinsics::copy
intrinsics::copy_nonoverlapping

can be said to have names different enough from their C counterparts. (YMMV of course, as this is a case where we can argue either way.)

(Also, if we rename them to ptr::copy_ft etc, people may ask, why copy in std::io is not copy_ft? Surely they have the same argument order?)

So maybe renaming won't be a worthy improvement, as long as people are aware of Rust's rule: src always comes first.

@liigo
Copy link
Contributor

liigo commented Apr 7, 2015 via email

@liigo
Copy link
Contributor

liigo commented Apr 7, 2015

I mean, please explain why you change it, in git commit message at least.
On Apr 7, 2015 8:59 PM, "Liigo Zhuang" com.liigo@gmail.com wrote:

-1
keep consistent in rust is more important than in c. Please stop doing
this change even without any reason.

@bfops
Copy link
Contributor

bfops commented Apr 7, 2015

Maybe this is the worst of both worlds, but what if the intrinsics are left to be C ordering, and the higher Rusty wrappers are given the Rust ordering? It could definitely be confusing if you switched between the two, but in my head, an intrinsic says "I don't control what this function looks like, I just link to it".

That said, it would really only make sense if std::intrinsics had a "specifically LLVM intrinsics" section, and wasn't all viewed as "rustc intrinsics". In that case, I'd probably also be in favor of renaming them to match their LLVM counterparts.

@lpar
Copy link

lpar commented Apr 7, 2015

I'm not a Rust user yet, but as someone considering the language for future projects who has written C recently, I would much rather have Rust be consistent itself, than slavishly copy whatever choices C has made over the years.

@aturon
Copy link
Member

aturon commented Apr 7, 2015

Cross-posted from reddit:

It was absolutely a mistake to push the ordering change through without an RFC, and I apologize for signing off on it. It wound up causing some painful fallout, and it's clear that many have strong opinions about the order here that weren't taken into account. Of course, this kind of thing won't happen in the future, since we won't be able to make changes to stable APIs.

The main question now is whether, and how, we should reverse this change. I'm going to suggest that discussion on the next step happen in the ongoing discuss thread, where there are already a number of interesting suggestions. Let's try to reach a consensus there before taking action.

As such, I'm going to close this PR for now, to try to move discussion to a central place and come to a decision before taking an action like this one.

@aturon aturon closed this Apr 7, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.