Stream: t-compiler

Topic: review requests


pnkfelix (Mar 12 2019 at 10:12, on Zulip):

As an experiment, I'm inviting people to post review requests here. (And I'm going transcribe some recent ones that I saw in other streams/topics)

pnkfelix (Mar 12 2019 at 10:14, on Zulip):

(transcribed)

does anyone feel like an expert on infer?

specifically regarding the confidence to review https://github.com/rust-lang/rust/pull/59008

pnkfelix (Mar 12 2019 at 10:16, on Zulip):

(transcribed)

Also, does anybody wants to review https://github.com/rust-lang/rust/pull/58349 (resolve: Simplify import resolution for mixed 2015/2018 edition mode)?
I usually reassign anything assigned to pnkfelix, but in this case I'm not sure who'd be the proper candidate.

pnkfelix (Mar 12 2019 at 10:24, on Zulip):

(heh I transcribed the latter robotically, even after I had responded saying I would try to get around to the review today... ugh need more sleep or more coffee)

oli (Mar 12 2019 at 12:11, on Zulip):

No idea who to r? here. A review would be appreciated: https://github.com/rust-lang/rust/pull/59128 It's about emitting color codes in json diagnostics

davidtwco (Mar 12 2019 at 12:17, on Zulip):

It all looks reasonable to me, but I'm not familiar with that code at all.

davidtwco (Mar 12 2019 at 12:23, on Zulip):

(also, why hasn't highfive assigned someone?)

oli (Mar 12 2019 at 12:40, on Zulip):

because I screwed up and r?ed someone who doesn't have powers

Esteban Küber (Mar 14 2019 at 17:12, on Zulip):

@oli left a comment.

Esteban Küber (Mar 14 2019 at 17:13, on Zulip):

Anyone with good insights into impl Trait returns and lifetimes, can you take a look at this hack and see if reasonable?
https://github.com/rust-lang/rust/pull/58919/files

pnkfelix (Mar 14 2019 at 20:18, on Zulip):

it definitely seems like a hack (just from seeing how its inspecting the name to see 'static); if @nikomatsakis doesn't look at it by tomorrow morning, I will try to see if I can suggest something that isn't quite as hacky

Zoxc (Mar 16 2019 at 12:51, on Zulip):

I have a couple of PRs I'd like to get reviewed now that mw is unavailable. Turn HIR indexing into a query (contains Combine input and eval_always query types) and Define queries using a proc macro.

oli (Mar 19 2019 at 12:08, on Zulip):

I've been told @mw isn't available for review rn. @WG-mir-opt or others: Does someone want to review adding a new invocation of the branch simplification mir optimization? https://github.com/rust-lang/rust/pull/59290

davidtwco (Mar 19 2019 at 12:10, on Zulip):

Looks good to me, can r=me (or I'll r+) once Travis finishes.

centril (Mar 19 2019 at 14:08, on Zulip):

@davidtwco ;)

Santiago Pastorino (Mar 19 2019 at 14:18, on Zulip):

saw the PR before this messages :)

Santiago Pastorino (Mar 19 2019 at 14:18, on Zulip):

looked good to me too

Vadim Petrochenkov (Jun 30 2019 at 22:51, on Zulip):

Does anybody want to review "Support stability and deprecation checking for all macros"?
It's series of refactorings/cleanups after which stability and deprecation mostly become implemented automatically.

Simon Sapin (Jul 04 2019 at 16:27, on Zulip):

@centril In https://github.com/rust-lang/rust/pull/62330#discussion_r300064460, are you asking for more tests? Doing what exactly? What this test does in the first place is not clear to me (even with the comment)

centril (Jul 04 2019 at 16:29, on Zulip):

@Simon Sapin basically add:

const X: () = (NoDrop { inner: ManuallyDrop::new(NeedDrop) }, ()).1;

const fn _f() { (NoDrop { inner: ManuallyDrop::new(NeedDrop) }, ()).1 }
centril (Jul 04 2019 at 16:30, on Zulip):

My understanding of this is that it is testing that since destructors are not run, it's fine inside a const context

centril (Jul 04 2019 at 16:30, on Zulip):

and specifically how ManuallyDrop and friends achieve this

Simon Sapin (Jul 04 2019 at 16:31, on Zulip):

Ok

centril (Jul 04 2019 at 16:31, on Zulip):

Thanks; and ~good night-ish ;) (very tired after 1.36.0 release, hehe)

pnkfelix (Jul 11 2019 at 09:18, on Zulip):

hey @eddyb , do you want to review this change to print_def_path: #62503 ?

eddyb (Jul 11 2019 at 09:19, on Zulip):

yeah it's on my catch-up list. I'll look at it now though

pnkfelix (Jul 11 2019 at 09:19, on Zulip):

okay, its not urgent (as in, doesn't need to be done this minute).

pnkfelix (Jul 11 2019 at 09:20, on Zulip):

I just figured since I'm going on leave after tomorrow, I should try to get my open PR's dealt with.

eddyb (Jul 11 2019 at 09:21, on Zulip):

ughhhh cyclicity strikes again

pnkfelix (Jul 11 2019 at 09:22, on Zulip):

by the way, I'm not clear on what the bool in the return value represents

pnkfelix (Jul 11 2019 at 09:22, on Zulip):

I'd be happy to add a comment explaining it if you want to enlighten me. :)

eddyb (Jul 11 2019 at 09:22, on Zulip):

I thought there was a comment?

pnkfelix (Jul 11 2019 at 09:23, on Zulip):

I didn't see one explaining the boolean

pnkfelix (Jul 11 2019 at 09:23, on Zulip):

its possible I overlooked it

eddyb (Jul 11 2019 at 09:23, on Zulip):

oh I forgot to update the comment. so, Self is always returned because of how that whole API works, and the bool is the actual result of the try_...

eddyb (Jul 11 2019 at 09:23, on Zulip):

the answer to the If possible in the doc comment

pnkfelix (Jul 11 2019 at 09:24, on Zulip):

Oh okay

pnkfelix (Jul 11 2019 at 09:24, on Zulip):

to be fair, it does say "and returns true"

eddyb (Jul 11 2019 at 09:24, on Zulip):

oh, I see, the comment still talks about returning a path, but it should say "prints" instead of "returns"

eddyb (Jul 11 2019 at 09:24, on Zulip):

then the "and returns true" makes more sense

pnkfelix (Jul 11 2019 at 09:25, on Zulip):

yeah I should fix that too. :smile:

eddyb (Jul 11 2019 at 09:42, on Zulip):

@pnkfelix btw if you want to take a look at #62584 - took me a while to submit that fix, mostly because I wasn't sure how to properly explain the math at work here

eddyb (Jul 11 2019 at 09:42, on Zulip):

"rotating a range" sounds weird but it's pretty much what's happening

pnkfelix (Jul 11 2019 at 09:44, on Zulip):

Okay I'll see if I can get to it; I have some other stuff on my plate to take care of in the real world in the near term (we're moving offices and I still need to pack up my desk)

eddyb (Jul 11 2019 at 09:46, on Zulip):

don't worry about it then

pnkfelix (Jul 11 2019 at 10:33, on Zulip):

ah I'm reading it anyway

pnkfelix (Jul 11 2019 at 10:33, on Zulip):

who needs to pack

eddyb (Jul 12 2019 at 14:50, on Zulip):

@pnkfelix if you're still around, I'm curious if you're okay with this silly approach to having a lot of variants https://github.com/rust-lang/rust/pull/62584/files#diff-dac0b3d5a19b291538d6f171b7e8eb8bR16

pnkfelix (Jul 12 2019 at 14:50, on Zulip):

:scream:

pnkfelix (Jul 12 2019 at 14:51, on Zulip):

not silly

pnkfelix (Jul 12 2019 at 14:51, on Zulip):

looks great to me

eddyb (Jul 12 2019 at 14:51, on Zulip):

(FWIW it does hit the second unreachable! as well on current Rust, so you were right to ask for this test)

eddyb (Jul 12 2019 at 14:51, on Zulip):

(and the PR does fix it)

pnkfelix (Jul 12 2019 at 14:52, on Zulip):

/me just finished packing. Took ... almost three hours

eddyb (Jul 12 2019 at 14:52, on Zulip):

I was gonna do hundreds of lines but then I remembered hex packs nicely :P

pnkfelix (Jul 12 2019 at 14:52, on Zulip):

(I had a lot of physical garbage debt)

matklad (Jul 20 2019 at 10:11, on Zulip):

I'd appreciate a review of https://github.com/rust-lang/rust/pull/59706 :)

simulacrum (Jul 20 2019 at 11:23, on Zulip):

@matklad Could you perhaps update the PR description? It still has some seemingly outdated language (draft PR, etc.)

Last update: Nov 22 2019 at 04:55UTC