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

utils: Add CodeMapExt extension trait for span_* methods #857

Merged
merged 1 commit into from
Mar 14, 2016

Conversation

kamalmarhubi
Copy link
Contributor

This commit adds a CodeMapExt extension trait on CodeMap, and moves some
functions to methods there:

  • span_after
  • span_after_last
  • span_before

This better reflects them being lookup methods on the codemap.

@nrc
Copy link
Member

nrc commented Mar 9, 2016

I like the refactoring. Is there any need for a trait here? It doesn't seem to make sense for anything other than a CodeMap to implement this stuff, so maybe it is better to just make the methods inherent methods on the CodeMap? If you do think we should keep the trait, I would like a more descriptive name than CodeMapExt CodeMapSpanUtils or something.

@kamalmarhubi
Copy link
Contributor Author

It doesn't seem to make sense for anything other than a CodeMap to implement this stuff, so maybe it is better to just make the methods inherent methods on the CodeMap?

CodeMap comes from syntex_syntax so I think we have to add things via a trait?

I would like a more descriptive name than CodeMapExt CodeMapSpanUtils or something.

I'm not sure about SpanUtils in the name, as I have a few extras that would end up in here as part of working on the line ranges stuff:

pub struct LineRange {
    pub lo: usize,
    pub hi: usize,
}

pub trait CodeMapExt {
    ...;
    fn lookup_line_range(&self, span: Span) -> LineRange;
}

In my branch where I'm working on that, I actually created a codemap module, but I'm not super happy with that factoring. Very happy to hear suggestions!

@kamalmarhubi
Copy link
Contributor Author

I guess there's no reason that could't be CodeMapLineRangeUtils or something?

@nrc
Copy link
Member

nrc commented Mar 13, 2016

Ok, that makes sense, sorry I got confused with something else.

CodeMapLineRangeUtils sounds good to me.

This commit adds a CodeMapSpanUtils extension trait on CodeMap, and
moves some functions to methods there:
  - span_after
  - span_after_last
  - span_before

This better reflects them being lookup methods on the codemap.
@kamalmarhubi
Copy link
Contributor Author

Sounds good. I've updated it to be CodeMapSpanUtils for these methods, and will add another CodeMapLineRangeUtils once that's added.

nrc added a commit that referenced this pull request Mar 14, 2016
utils: Add CodeMapExt extension trait for span_* methods
@nrc nrc merged commit da65090 into rust-lang:master Mar 14, 2016
@kamalmarhubi kamalmarhubi deleted the codemap-ext branch April 1, 2016 20:41
kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this pull request Apr 21, 2016
This commit adds a `codemap` module, and moves the `CodemapSpanUtils`
added in rust-lang#857 to it. This is preparation for adding more `Codemap`
specific utilities.

Refs rust-lang#434
kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this pull request May 26, 2016
This commit adds a `codemap` module, and moves the `CodemapSpanUtils`
added in rust-lang#857 to it. This is preparation for adding more `Codemap`
specific utilities.

Refs rust-lang#434
kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this pull request May 26, 2016
This commit adds a `codemap` module, and moves the `CodemapSpanUtils`
added in rust-lang#857 to it. This is preparation for adding more `Codemap`
specific utilities.

Refs rust-lang#434
kamalmarhubi added a commit to kamalmarhubi/rustfmt that referenced this pull request May 30, 2016
This commit adds a `codemap` module, and moves the `CodemapSpanUtils`
added in rust-lang#857 to it. This is preparation for adding more `Codemap`
specific utilities.

Refs rust-lang#434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants