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

Attempted fix to issue #63551 #63906

Open
wants to merge 1 commit into
base: master
from

Conversation

@luigishat
Copy link

commented Aug 26, 2019

Well this is my first PR so hopefully I didn't do too bad.

As said I'm attempting to fix issue #63551 and remove the unnecessary check for is_unit.

would recommend removing let is_unit = input_ty.is_unit() unless there is another use.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-26T02:14:21.2411493Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-26T02:14:21.2610545Z ##[command]git config gc.auto 0
2019-08-26T02:14:21.2684906Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-26T02:14:21.2738554Z ##[command]git config --get-all http.proxy
2019-08-26T02:14:21.2872112Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63906/merge:refs/remotes/pull/63906/merge
---
2019-08-26T02:14:56.0302018Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-26T02:14:56.0302053Z 
2019-08-26T02:14:56.0302329Z   git checkout -b <new-branch-name>
2019-08-26T02:14:56.0302648Z 
2019-08-26T02:14:56.0302723Z HEAD is now at 608fcfb35 Merge 8bb577ff9e907400314029a87ebf49072ca1338f into 521d78407471cb78e9bbf47160f6aa23047ac499
2019-08-26T02:14:56.0489012Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-26T02:14:56.0492274Z ==============================================================================
2019-08-26T02:14:56.0492650Z Task         : Bash
2019-08-26T02:14:56.0492741Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-26T02:21:34.0035329Z    Compiling serde_json v1.0.40
2019-08-26T02:21:35.8555794Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-26T02:21:46.7754332Z     Finished release [optimized] target(s) in 1m 30s
2019-08-26T02:21:46.7836354Z tidy check
2019-08-26T02:21:46.9976993Z tidy error: /checkout/src/librustc_codegen_ssa/mir/rvalue.rs:589: trailing whitespace
2019-08-26T02:21:48.9713099Z some tidy checks failed
2019-08-26T02:21:48.9714239Z 
2019-08-26T02:21:48.9714239Z 
2019-08-26T02:21:48.9715247Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-26T02:21:48.9715422Z 
2019-08-26T02:21:48.9715449Z 
2019-08-26T02:21:48.9715782Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-26T02:21:48.9715849Z Build completed unsuccessfully in 0:01:33
2019-08-26T02:21:48.9715849Z Build completed unsuccessfully in 0:01:33
2019-08-26T02:21:48.9759982Z == clock drift check ==
2019-08-26T02:21:48.9775282Z   local time: Mon Aug 26 02:21:48 UTC 2019
2019-08-26T02:21:49.1331822Z   network time: Mon, 26 Aug 2019 02:21:49 GMT
2019-08-26T02:21:49.1335991Z == end clock drift check ==
2019-08-26T02:21:50.4824763Z ##[error]Bash exited with code '1'.
2019-08-26T02:21:50.4858164Z ##[section]Starting: Checkout
2019-08-26T02:21:50.4860250Z ==============================================================================
2019-08-26T02:21:50.4860314Z Task         : Get sources
2019-08-26T02:21:50.4860368Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-26T04:11:15.3229162Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-26T04:11:16.2175925Z ##[command]git config gc.auto 0
2019-08-26T04:11:16.2180610Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-26T04:11:16.2183097Z ##[command]git config --get-all http.proxy
2019-08-26T04:11:16.2187942Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63906/merge:refs/remotes/pull/63906/merge
---
2019-08-26T04:11:52.2661544Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-26T04:11:52.2661567Z 
2019-08-26T04:11:52.2661749Z   git checkout -b <new-branch-name>
2019-08-26T04:11:52.2662803Z 
2019-08-26T04:11:52.2663588Z HEAD is now at 019ba58cc Merge 4ab65c6a993e82a85c8ce50aa52a7215e53a5b41 into 4c58535d09d1261d21569df0036b974811544256
2019-08-26T04:11:52.2816432Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-26T04:11:52.2819474Z ==============================================================================
2019-08-26T04:11:52.2819696Z Task         : Bash
2019-08-26T04:11:52.2819865Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-26T04:42:46.3754255Z    Compiling semver v0.9.0
2019-08-26T04:42:47.8045566Z error: failed to run custom build command for `libc v0.2.61`
2019-08-26T04:42:47.8045677Z 
2019-08-26T04:42:47.8045727Z Caused by:
2019-08-26T04:42:47.8046283Z   process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/build/libc-b24c65ec356e6fe9/build-script-build` (signal: 11, SIGSEGV: invalid memory reference)
2019-08-26T04:42:48.1971967Z error: build failed
2019-08-26T04:42:48.2007703Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2019-08-26T04:42:48.2008046Z expected success, got: exit code: 101
2019-08-26T04:42:48.2017509Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-26T04:42:48.2017509Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-26T04:42:48.2017586Z Build completed unsuccessfully in 0:24:38
2019-08-26T04:42:48.2070109Z == clock drift check ==
2019-08-26T04:42:48.2085072Z   local time: Mon Aug 26 04:42:48 UTC 2019
2019-08-26T04:42:48.3578557Z   network time: Mon, 26 Aug 2019 04:42:48 GMT
2019-08-26T04:42:48.3581470Z == end clock drift check ==
2019-08-26T04:42:49.5777899Z ##[error]Bash exited with code '1'.
2019-08-26T04:42:49.5842012Z ##[section]Starting: Checkout
2019-08-26T04:42:49.5844336Z ==============================================================================
2019-08-26T04:42:49.5844408Z Task         : Get sources
2019-08-26T04:42:49.5844456Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Could you please remove the rustfmt commit? While it is ok to format the lines you are working with, fornatting the rest gives too much noise.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

r? @bjorn3
cc @eddyb

@rust-highfive rust-highfive assigned bjorn3 and unassigned petrochenkov Aug 26, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@bors delegate=bjorn3
(Not sure all assignable people have r+ rights.)

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

✌️ @bjorn3 can now approve this pull request

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@petrochenkov I am not part of the rust team.

r? @eddyb (does that work?)

@rust-highfive rust-highfive assigned eddyb and unassigned bjorn3 Aug 26, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@bjorn3

I am not part of the rust team.

(That's not too important, I know that you worked on rustc_codegen_ssa and familiar with the code, so you can review.)

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

(That's not too important, I know that you worked on rustc_codegen_ssa and familiar with the code, so you can review.)

Didn't know that. Is that written down somewhere as policy?

@Kixiron

This comment has been minimized.

Copy link

commented Aug 26, 2019

@luigishat Tests are not currently passing

@luigishat

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@Kixiron and @bjorn3 Take another look please I think I did what you wanted.

@@ -546,7 +546,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
) -> Bx::Value {
let is_float = input_ty.is_floating_point();
let is_signed = input_ty.is_signed();
let is_unit = input_ty.is_unit();
// No need for this anymore ISSUE:#63551
let _is_unit = input_ty.is_unit();

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Aug 28, 2019

Contributor

Please remove this line and the one above.

})
} else if is_float {
mir::BinOp::Eq | mir::BinOp::Le | mir::BinOp::Ge => {
if is_float {

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Aug 28, 2019

Contributor

Please remove the { on the line above and merge this line into the one above.

@@ -600,10 +596,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
base::bin_op_to_icmp_predicate(op.to_hir_binop(), is_signed),
lhs, rhs
)
}

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Aug 28, 2019

Contributor

Please remove this line.

}
}
}


This comment has been minimized.

Copy link
@bjorn3

bjorn3 Aug 28, 2019

Contributor

Please remove this newline.

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Please squash your comments after fixing the review comments above. See for example http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html for how to squash.

@luigishat

This comment has been minimized.

Copy link
Author

commented Aug 29, 2019

Sorry Having trouble with git rebase I have a merge conflict with rvalue.rs and It's giving me a headache.

@luigishat

This comment has been minimized.

Copy link
Author

commented Aug 31, 2019

@bjorn3 I tried to git squash it but I think I messed it up. When I finish with git rebase I keep getting thrown back a commit and am required to pull and henceforth get stuck pushing two commits at once ("Added two commits six days ago"). Help would be appreciated

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

Maybe reset your branch back to master and perform you changes again.

$ git fetch origin # get latest version of rust
$ git reset --hard origin/master # reset current branch back to latest version of rust (throws all changes away)
$ # perform changes and commit
$ git push --force # push your changes to your fork

Hope that helps. Over time you will probably get better at using git, but at first it can be hard.

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 7, 2019

Ping from triage:
@luigishat can you please address the comments here? thanks!

@luigishat luigishat force-pushed the luigishat:cmp_fix branch from 2b688a8 to 4b57287 Sep 9, 2019

@luigishat

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

@JohnCSimon @bjorn3 Sorry about the late action I've had problems with my computer and just got my self repairs done today. Thanks for being patient 👍

@bjorn3
bjorn3 approved these changes Sep 9, 2019
@luigishat

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

So what now @bjorn3 ?

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I dont think the delegate to me (#63906 (comment)) os still active because of #63906 (comment), so you will have to wait for @eddyb or someone else with permission to accept a PR.

@luigishat

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@bjorn3 @Kixiron could you try r+ because right now I think this PR is stagnant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.