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

Constants for mutability that are useful for visitor api are in libsyntax #3262

Closed
dbp opened this issue Aug 23, 2012 · 5 comments
Closed

Constants for mutability that are useful for visitor api are in libsyntax #3262

dbp opened this issue Aug 23, 2012 · 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.

Comments

@dbp
Copy link
Contributor

dbp commented Aug 23, 2012

This is not good, as it means anything that wants to use the visitor api (for example, I'm working on having fmt! "%?" use it) would depend on libsyntax.

@brson
Copy link
Contributor

brson commented Aug 24, 2012

Which constants?

@dbp
Copy link
Contributor Author

dbp commented Aug 24, 2012

Sorry I should have clarified what I meant by "Constants for mutability": In syntax/ast.rs:

181: enum mutability { m_mutbl, m_imm, m_const, }

The reason why this is an issue is that there are functions in the reflection api, like for example:

fn visit_rec_field(_i: uint, _name: &str,  _mtbl: uint, inner: *tydesc) -> bool

where _mtbl is the uint value of that enum. This actually really confused me at first, because I figured it was a boolean cast as a uint, but 0 was mutable, and 1 was immutable (and 2 const, but I don't think that's relevant for record fields?)! Ideally the visitor api would actually have the function have this signature:

fn visit_rec_field(_i: uint, _name: &str,  _mtbl: mutability, inner: *tydesc) -> bool

And mutability would live somewhere that could be included by core::reflect (once that exists) as well as libsyntax.

There may be other constants like this as well - I've only just started working with the api.

@catamorphism
Copy link
Contributor

Not critical for 0.6; de-milestoning

@emberian
Copy link
Member

emberian commented Aug 5, 2013

I don't think this is worth fixing; it looks to me like the TyVisitor needs some serious love (it has methods like visit_class_field), and any changes to it would ideally use a safer interface than magic numbers.

@thestinger
Copy link
Contributor

I don't think this will be a problem, because const is being removed. We can switch the mtbl parameter to a simple bool.

RalfJung pushed a commit to RalfJung/rust that referenced this issue Jan 22, 2024
Run the tier 2 sysroots job on a schedule, not push

I had this running on push instead of on a schedule so that I could verify that it worked, then forgot to switch it back to cron before merging.
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.
Projects
None yet
Development

No branches or pull requests

5 participants