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

Add convenience wrappers for type_dependent_defs #59094

Closed
3 tasks
oli-obk opened this issue Mar 11, 2019 · 5 comments
Closed
3 tasks

Add convenience wrappers for type_dependent_defs #59094

oli-obk opened this issue Mar 11, 2019 · 5 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2019

Most uses of that method mostly directly unwrap or just want to know the DefId. So we could use several kind of convenience wrappers:

  • fn opt_type_dependent_def(&self, id: HirId) -> Option<Def>
  • fn type_dependent_def(&self, id: HirId) -> Def
  • fn type_dependent_def_id(&self, id: HirId) -> DefId
@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Mar 11, 2019
@stepnivlk
Copy link
Contributor

hey @oli-obk, happy to take this one if its still available

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 11, 2019

Wow that was fast! Yes of course! If you have any questions, don't hesitate to ask here or on discord/zulip.

@stepnivlk
Copy link
Contributor

@oli-obk Hi again :).
So I started implementing proposed methods and updating calls to them over the codebase and I noticed one def_id extracting pattern when there's something like this:

if let Some(def) = self.tables.type_dependent_defs().get(expr.hir_id) {
  let def_id = def.def_id();
  // do something with `def_id`
} else {
  // do something else
}

It seems to me like we can introduce fourth method:
fn opt_type_dependent_def_id(&self, id: HirId) -> Option<DefId>

Allowing us to rewrite previous example as:

if let Some(def_id) = self.tables.opt_type_dependent_def_id(expr.hir_id) {
  // do something with `def_id`
} else {
  // do something else
}

Leading to little bit cleaner code. Let me know what you think.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 12, 2019

Good idea!

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Mar 23, 2019
…ers, r=oli-obk

Type dependent defs wrappers

First of all, forgive me if something would seem lame to you or I offend some rule (although I tried to read through docs), this is my first PR.

Issue: rust-lang#59094

This PR adds 3 helper methods to `TypeckTables`:
* `opt_type_dependent_def`
* `opt_type_dependent_def_id`
* `type_dependent_def_id`

I didn't add `type_dependent_def` as was proposed in the issue simply because it wasn't used anywhere in the code. Only non-option wrapped`type_dependent_defs()[]` accesses were found in clippy which always called `def_id()` on result.
Speaking of clippy, should I open separate PR in its own repo, given it's used as submodule here?

Sry it took me so long, as I said I'm new here and I had tough week :).
Centril added a commit to Centril/rust that referenced this issue Mar 28, 2019
…ers, r=oli-obk

Type dependent defs wrappers

First of all, forgive me if something would seem lame to you or I offend some rule (although I tried to read through docs), this is my first PR.

Issue: rust-lang#59094

This PR adds 3 helper methods to `TypeckTables`:
* `opt_type_dependent_def`
* `opt_type_dependent_def_id`
* `type_dependent_def_id`

I didn't add `type_dependent_def` as was proposed in the issue simply because it wasn't used anywhere in the code. Only non-option wrapped`type_dependent_defs()[]` accesses were found in clippy which always called `def_id()` on result.
Speaking of clippy, should I open separate PR in its own repo, given it's used as submodule here?

Sry it took me so long, as I said I'm new here and I had tough week :).
@wesleywiser
Copy link
Member

This can be closed now right?

@oli-obk oli-obk closed this as completed Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

3 participants