store ADT information in an AdtDef structure in a unified way

[arielb1]

This ended up being a bigger refactoring than I thought, as I also cleaned a few ugly points in rustc. There are still a few areas that need improvements.

Performance numbers:

Before:
572.70user 5.52system 7:33.21elapsed 127%CPU (0avgtext+0avgdata 1173368maxresident)k
llvm-time: 385.858

After:
545.27user 5.49system 7:10.22elapsed 128%CPU (0avgtext+0avgdata 1145348maxresident)k
llvm-time: 387.119

A good 5% perf improvement. Note that after this patch >70% of the time is spent in LLVM - Amdahl's law is in full effect.

Passes make check locally.

r? @nikomatsakis

[eefriedman]

I assume this slipped in by accident?

[arielb1]

yes

[arielb1]

passes check locally

[Aatch]

Awesome, I'm glad somebody did this. The performance increase is consistent with what I saw with my version of this change. Hopefully this should also make working with ADTs easier going forward, at the very least it's much more obvious.

[nikomatsakis]

@arielb1

I copied somewhere that did .0 (you can't believe how many places do this). I match on the binder in several places - maybe I'll add a tuple_ctor_to_type function.

Oh, I can believe it, since I wrote them, but I've been migrating code to the following conventions:

1. Either use a proper function (like no_late_bound_regions) or,
2. If you call skip_binder, add a comment explaining why it makes sense in this particular case.

[nikomatsakis]

@arielb1

Having AdtDef<'tcx> be &'tcx AdtDef<'tcx,'tcx> is certainly nice. AdtDef_ is used basically nowhere (once in the tcx, once in decoder, once in collect), I'm not sure it should be behind a type alias. Also, this isn't really Mut - you can still fulfill an AdtDef<'tcx> with a Ty<'static>.

While technically true, that's not particularly interesting, is it? The intention of these types is as I wrote them, no? I'm fine if you want to note that caveat in a comment, but I still favor names that lay clear the intent of "read-only" vs "mutable", as this is a subtle trick.

[nikomatsakis]

@arielb1

dropck was somewhat an hotspot on my profile.

How is this relevant? I'm talking about what module the code is in, not what it does.

[nikomatsakis]

@Aatch

that's for the entire patchset up to this point (I wanted to separate it from Ty::is_simd).

ah, ok, sounds good.

[nikomatsakis]

cc @rust-lang/compiler <-- this PR is worth having more eyes on, incidentally

[nikomatsakis]

OK, I've finished reading through the PR. I am happy with it, this is nice work.

r+ modulo:

• rename AdtDef_ to AdtDefData or AdtDefStruct or something similar with a word, not an underscore
• readable type aliases for the (mostly) "read-only" &'tcx AdtDefData<'tcx,'static> and the mutable &'tcx AdtDefData<'tcx,'tcx>. Personally, I think it's worth giving both of them an alias because it makes the pattern clearer, even if the latter gets little use.
• remove or at minimum document calls to skip_binder
• explain where the T: Copy bound comes from (lets us use Cell; since we store &T it's fine; could be lifted in the future but would require more unsafe code). I was thinking that it mattered for concurrency but of course I was being silly: the whole POINT of an ivar is that it's write-once, and hence you can clone the contents without fear of them being overwritten, after all.

[nikomatsakis]

ps, if you prefer, perhaps the name AdtDefTcx or something would be a compromise vs AdtDefMut ("fulfillable with a Ty<'tcx>")

[arielb1]

addressed

[nikomatsakis]

@bors r+

[bors]

📌 Commit eedb1cc has been approved by nikomatsakis

[bors]

⌛ Testing commit eedb1cc with merge ab77c1d...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt