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

Fix the span of generated #[derive_*] attributes #33926

Merged
merged 4 commits into from
May 30, 2016

Conversation

jseyfried
Copy link
Contributor

Fixes #33571.
r? @nrc

@jseyfried
Copy link
Contributor Author

c.f. #32791 (specifically this commit), which caused #33571.
cc @nikomatsakis @LeoTestard

//
// See tests src/run-pass/rfc1445 for
// examples. --nmatsakis
let span = Span { expn_id: cx.backtrace(), .. span };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it still necessary to have a special case for the PartialEq and Eq traits? Is the span generated in the loop above not sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed.

// originate not from the deriving call but from
// text outside the deriving call, and hence would
// be forbidden from using unstable
// content.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, uh, forget what I just said. This comment explains why this case indeed needs a special span.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errmmm... Or maybe not, since the span taht we compute in the loop has allow_internal_unstable set? I must admit I'm a bit confused.

Copy link
Contributor

@LeoTestard LeoTestard May 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well okay, no, now that you eplicitly set allow_internal_unstable, which wasn't the case before I think, it should be okay. But you should definitely add a test like what the comment suggests and make sure it compiles. Something like:

macro_rules! foo() (
    ($attr:tt) => (
        #[deriving($attr, PartialEq)] struct S;
    )
);

foo!(Eq);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fine. Looks good to me then. :)

@nrc
Copy link
Member

nrc commented May 30, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 30, 2016

📌 Commit a0e606c has been approved by nrc

Manishearth added a commit to Manishearth/rust that referenced this pull request May 30, 2016
Fix the span of generated `#[derive_*]` attributes

Fixes rust-lang#33571.
r? @nrc
bors added a commit that referenced this pull request May 30, 2016
Rollup of 5 pull requests

- Successful merges: #33867, #33926, #33942, #33958, #33964
- Failed merges:
@bors bors merged commit a0e606c into rust-lang:master May 30, 2016
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.

4 participants