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.

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

Jump to bottom

First person third arrays #11

Merged
merged 10 commits into from Aug 6, 2017
Merged

First person third arrays #11

merged 10 commits into from Aug 6, 2017

Conversation

gurgalex
Copy link
Contributor

@gurgalex gurgalex commented Aug 4, 2017 •
edited

Working toward #8
Each pull request will focus on one file.

Reworded some more sentences.
Although, in some cases I reworded a paragraph to reduce the amount of "you", or to make a sentence more straightforward.

Michael-F-Bryan
Michael-F-Bryan requested changes Aug 4, 2017
View reviewed changes
Copy link
Owner

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

Looks great! I think there's only one or two spots which I added comments to, so once they're dealt with this will be good to merge.

src/arrays/index.md Outdated
@@ -129,13 +128,13 @@ libaverages.a(std-9a66b6a343d52844.0.o): In function `std::sys::imp::mutex::{{im
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

Oops! When clang tried to compile our `libaverages.a` library and `main.c` into
one executable it wasn't able to find a bunch of symbols.
Oops! When clang tried to compile the libraries `libaverages.a` and `main.c`
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if "the libraries" is the right wording here. Technically only libaverages.a is a library while main.c is a program...

What about going with the original paragraph and just removing the "our" on the first line?

Copy link
Contributor Author

@gurgalex gurgalex Aug 4, 2017 •
edited

Choose a reason for hiding this comment

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

What about

  1. "... compile the library libaverages.a and main.c program into one executable"

That way it is made clear that one is a library, and the other is a program.

Copy link
Owner

Choose a reason for hiding this comment

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

I like it

EDIT: Ignore this. What you propose in this later comment sounds better.

src/arrays/index.md Outdated
about. When you compile everything statically you need to include **all** your
dependencies. You didn't have this issue when dynamically linking because the
loader finds everything for you.
Remember those notes from earlier? That's what `rustc` was trying to warn
Copy link
Owner

Choose a reason for hiding this comment

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

Do you reckon we should add a link so when you click "those notes" it'll go back to the notes it was referring to. Like you mention in #10

Copy link
Contributor Author

@gurgalex gurgalex Aug 4, 2017 •
edited

Choose a reason for hiding this comment

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

Yup, so I can either,

  1. Add the notes link commit from Link back to rustc's notes on static compiling #10, and close the Link back to rustc's notes on static compiling #10 pull request.
  2. Wait until this PR is merged and then update Link back to rustc's notes on static compiling #10 to be based off the updated master branch.

@gurgalex
Copy link
Contributor Author

gurgalex commented Aug 4, 2017 •
edited

Ah, I feared that merging #10 first would cause a merge conflict.
Um, how should I resolve that?

Update:
It was #13 that caused the merge conflict

@Michael-F-Bryan
Copy link
Owner

Michael-F-Bryan commented Aug 4, 2017 •
edited

It looks like GitHub automatically closed #10 when I merged #13. Can you rebase from master (so this branch is up to date with the latest changes git checkout master; git pull upstream master; git checkout first_person_third_arrays; git rebase master) then you'll need to open up the file in your editor and look for a bunch of >>>>. That shows the changes before and after, and all you need to do is try and merge the two versions of the paragraph.

Afterwards you need to do a force push back to GitHub because the GitHub version of this branch doesn't have the new changes as part of its history. It sounds more complicated than it actually is, usually git is really helpful in telling you what you need to do next.

gurgalex
gurgalex commented Aug 4, 2017
View reviewed changes
src/arrays/index.md Outdated
@@ -84,15 +84,14 @@ note: library: util
```

As well as compiling `averages.rs` into a static library, `rustc` has also
emitted some helpful notes which we'll need when linking the library into one
executable.
emitted some helpful notes needed for linking the library into one executable.
Copy link
Contributor Author

@gurgalex gurgalex Aug 4, 2017 •
edited

Choose a reason for hiding this comment

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

"linking the library into one executable" reads strangely to me.
Does "linking the library into an executable", still imply that we're linking into only one executable?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I agree. In that context "an executable" would be better than "one executable".

gurgalex
gurgalex commented Aug 4, 2017
View reviewed changes
src/arrays/index.md Outdated
Oops! When clang tried to compile our `libaverages.a` library and `main.c` into
one executable it wasn't able to find a bunch of symbols.
Oops! When clang tried to compile the `libaverages.a` library and `main.c`
program into one executable it wasn't able to find a bunch of symbols.
Copy link
Contributor Author

@gurgalex gurgalex Aug 4, 2017 •
edited

Choose a reason for hiding this comment

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

How does omitting the program and library word sound?
Since the library has a filename starting with "lib" and main.c does not.

"..tried to compile libaverages.a and main.c into one executable..."

Copy link
Owner

Choose a reason for hiding this comment

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

Actually yeah, that's probably the best way to word it. It's pretty obvious that libaverages.a is a library, and main.c usually holds the main() function for a program by convention anyway.

@Michael-F-Bryan
Copy link
Owner

This looks good to me, @gurgalex would you like me to merge it now?

@gurgalex
Copy link
Contributor Author

gurgalex commented Aug 6, 2017

Yes

@gurgalex
Copy link
Contributor Author

gurgalex commented Aug 6, 2017 •
edited

I found some instances of lets that are actually let's in disguise. Thus they should be reworded.
If it's not too late, cancel my yes on merging for now.
Or, I can put them in another pull request.

@Michael-F-Bryan
Copy link
Owner

How about I merge this one and you can create a new issue in the issue tracker for the "lets"? That way we won't forget about it and it could also be an easy first commit for someone else if you or I don't get to it first.

@Michael-F-Bryan Michael-F-Bryan merged commit 29a286d into Michael-F-Bryan:master Aug 6, 2017
@gurgalex gurgalex deleted the first_person_third_arrays branch August 6, 2017 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Michael-F-Bryan Michael-F-Bryan Michael-F-Bryan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gurgalex @Michael-F-Bryan