Stream: t-compiler

Topic: #50592 extract bit-related types to crate


Flavius Aspra (Jun 25 2019 at 15:57, on Zulip):

Hi. I'm Flavius, wrote on rust/issues/50592 "extract SparseBitMatrix, SparseBitSet, and other bit-related types into a rust-lang-nursery crate".

I've been snooping around the code and git log. It looks like a rather easy task, minus the rustc-specific parts. Similar to how "ena" was reused.

What are the steps? For a start, I'd create a local library, move the code over to lib.rs, and use path in dependencies locally, fix the rest of the imports in the code.

Anything else?

How to share my progress? I'd have to create a repository under my github account, but at some point that will have to end up in rust-lang-nursery. Or is there another order of operations, like someone creates a repository and I push there?

centril (Jun 25 2019 at 15:58, on Zulip):

cc @nikomatsakis ^---

centril (Jun 25 2019 at 15:58, on Zulip):

Link: #50592

Flavius Aspra (Jun 25 2019 at 16:00, on Zulip):

cc nikomatsakis ^---

Thanks for the edit (I guess). I'm new to zulip as well. I'm getting too old to keep up with all the new tech I guess.

Flavius Aspra (Jun 25 2019 at 16:03, on Zulip):

Grepping through the code revealed:

bit_set.rs:27:pub struct BitSet<T: Idx> {
bit_set.rs:294:pub struct BitIter<'a, T: Idx> {
bit_set.rs:348:pub struct SparseBitSet<T: Idx> {
bit_set.rs:644:pub struct GrowableBitSet<T: Idx> {
bit_set.rs:695:pub struct BitMatrix<R: Idx, C: Idx> {
bit_set.rs:886:pub struct SparseBitMatrix<R, C>

and

bit_set.rs:453:pub enum HybridBitSet<T: Idx> {

anything else which should be extracted?

nikomatsakis (Jun 25 2019 at 16:30, on Zulip):

Hi @Flavius Aspra =) Man, I forgot about that issue. We've since created a kind of "checklist" for extracting things into crates.

nikomatsakis (Jun 25 2019 at 16:30, on Zulip):

Detailed here

nikomatsakis (Jun 25 2019 at 16:30, on Zulip):

Per that policy, we may want to start by getting agreement that this is a good idea =)

nikomatsakis (Jun 25 2019 at 16:31, on Zulip):

Although I still think it'd be useful

Flavius Aspra (Jun 25 2019 at 16:32, on Zulip):

How do we get agreement? What's the process?

Flavius Aspra (Jun 25 2019 at 16:33, on Zulip):

Oh, pull request for that document?

Flavius Aspra (Jun 25 2019 at 16:35, on Zulip):

Going through the document now, but yes, the first issue I am having is naming the new crate.

nagisa (Jun 25 2019 at 16:38, on Zulip):

Just saying: in the past extracting such API ended up in suboptimal API for general use – the APIs in rustc are usually tailored for rustc’s use alone

nagisa (Jun 25 2019 at 16:39, on Zulip):

I would re-implement these types from scratch, honestly. You will get much nicer results when not considering rustc as a single user of the API.

Flavius Aspra (Jun 25 2019 at 16:44, on Zulip):

I've just skimmed through and it looks reasonably reusable. There might be some spots here and there, but generally... Could you give an example of a suboptimal piece of code for generic use? https://github.com/rust-lang/rust/blob/master/src/librustc_data_structures/bit_set.rs

nikomatsakis (Jun 25 2019 at 19:52, on Zulip):

One thing I will say is that I commonly miss these types when working on other crates

nikomatsakis (Jun 25 2019 at 19:52, on Zulip):

though it's true that when I recreate them I often do some things differently

nikomatsakis (Jun 25 2019 at 19:52, on Zulip):

(sometimes just avoiding nightly features)

nikomatsakis (Jun 25 2019 at 19:52, on Zulip):

there exists already some crate that tried to extract index-vec I think and a few related types

nikomatsakis (Jun 25 2019 at 19:52, on Zulip):

but I didn't like how they did it, I forget why

Flavius Aspra (Jun 25 2019 at 20:00, on Zulip):

@nikomatsakis Ok, So I'll send a PR for that document first to kick off the discussion. The ticket says to put the new repository in rust-lang-nursery, and the document says rust-lang. Which one is right?

Flavius Aspra (Jun 25 2019 at 20:22, on Zulip):

I hope I got the process right so far https://github.com/rust-lang/compiler-team/pull/112

Flavius Aspra (Jun 26 2019 at 20:43, on Zulip):

Hi. Do you guys have any feedback for me? Anything at all would be great.

pnkfelix (Jun 27 2019 at 10:53, on Zulip):

Well, first of all, you need not (and should not) issue any command to rfcbot there. (update: I may have been wrong about this.)

pnkfelix (Jun 27 2019 at 10:57, on Zulip):

i'll assign myself as a reviewer in the hopes that I will remember to come back and review your PR either later today (unlikely) or tomorrow

pnkfelix (Jun 27 2019 at 10:59, on Zulip):

oh whoops I thought this was a PR on rust-lang/rust

pnkfelix (Jun 27 2019 at 11:00, on Zulip):

I'm pretty sure the new repository would go in rust-lang-nursery; the rust-lang area is reserved for a pretty limited set of projects.

centril (Jun 27 2019 at 11:01, on Zulip):

@pnkfelix nothing new should be added to rust-lang-nursery; the organization is deprecated

pnkfelix (Jun 27 2019 at 11:02, on Zulip):

okay well then I don't understand our process.

centril (Jun 27 2019 at 11:02, on Zulip):

Or at least I think so... cc @Pietro Albini

pnkfelix (Jun 27 2019 at 11:02, on Zulip):

I guess I'll go back and review the docs myself.

pnkfelix (Jun 27 2019 at 11:03, on Zulip):

Okay, it looks like I was wrong and these new crates are indeed going under rust-lang/

pnkfelix (Jun 27 2019 at 11:04, on Zulip):

@Flavius Aspra the main reason that I immediately reacted with "don't use rfcbot there" there is that it seemed premature to attempt a merge when there are unresolved Q's or decisions to be made.

centril (Jun 27 2019 at 11:06, on Zulip):

rfcbot only listens to team members anyways

Flavius Aspra (Jul 12 2019 at 02:08, on Zulip):

Hi. Can we nudge this? @pnkfelix

pnkfelix (Jul 12 2019 at 07:10, on Zulip):

Cc @nagisa since @nikomatsakis and I are going on PTO

nikomatsakis (Jul 12 2019 at 10:58, on Zulip):

my honest opinion @Flavius Aspra is that we should find you a different task :)

nikomatsakis (Jul 12 2019 at 10:59, on Zulip):

and close the issue

nikomatsakis (Jul 12 2019 at 10:59, on Zulip):

not because it's a bad idea, I suspect it's actually a good idea, but the question of how many crates to use and so forth is somewhat contentious and maybe just not the best use of time to resolve at this point.

nagisa (Jul 12 2019 at 13:09, on Zulip):

So @Flavius Aspra given that most of the bit stuff is already in a single crate, so extracting them should be limited to:

1. cargo new a crate;
2. Copy things you want to copy into your new crate;
3. Clean-up/expand the code/API to make it sensible as a standalone library.
With that you will have a crate to publicize. I’d post that somewhere on my own github if I was doing this. Even if we end up deciding we do not want it to be a rustc-maintained crate, you can post it on crates.io yourself as well. That’s how some of the crates split off rustc in the past.

nagisa (Jul 12 2019 at 13:13, on Zulip):

After that the code inside rustc can be removed and uses of bit structures would be replaced with those from the published crate.

nikomatsakis (Jul 12 2019 at 13:54, on Zulip):

Maybe I'm too negative, I suppose.

nikomatsakis (Jul 12 2019 at 13:54, on Zulip):

As I said, I still think it's a reasonable boundary to extract.

nikomatsakis (Jul 12 2019 at 13:54, on Zulip):

It occurs to me that one option -- before we move it to crates.io or whatever -- would be to just try and create a rustc_index crate

nikomatsakis (Jul 12 2019 at 13:54, on Zulip):

to collect the things and identify a "coherent set"

simulacrum (Jul 12 2019 at 13:59, on Zulip):

This is a great option for many reasons whenever it makes sense from an API boundary perspective

nikomatsakis (Jul 12 2019 at 14:22, on Zulip):

@Flavius Aspra I closed the issue above in favor of the first step being to create the crate in tree -- if you're down with that plan, I can leave some notes in the issue on how to do it!

nikomatsakis (Jul 12 2019 at 14:24, on Zulip):

Also, @Pietro Albini or @simulacrum or whoever maintains triagebot -- for some reason, attempting to close https://github.com/rust-lang/compiler-team/pull/112 seems to have created some kind of loop? 112 comments or so like this:


Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the master branch to enable it.

Please let @rust-lang/release know if you're having trouble with this bot.

Pietro Albini (Jul 12 2019 at 14:25, on Zulip):

killed the bot

simulacrum (Jul 12 2019 at 14:25, on Zulip):

yeah, investigating, thanks!

nikomatsakis (Jul 12 2019 at 14:25, on Zulip):

thanks y'all!

nikomatsakis (Jul 12 2019 at 14:29, on Zulip):

Also @Flavius Aspra left some mentoring notes here

pnkfelix (Jul 12 2019 at 19:41, on Zulip):

I've got 112 comments but #112 ain't one.

Flavius Aspra (Jul 30 2019 at 19:46, on Zulip):

Sorry guys, I'm switching from full-time contractor/freelancer to full-time employee, which also means changing in my lifestyle. I'm interested in rust and I'll probably come back for something to get started in a few months, but I'd rather not block your plans with this particular ticket.

nagisa (Jul 31 2019 at 00:23, on Zulip):

Good luck!

Last update: Nov 22 2019 at 04:55UTC