-
Notifications
You must be signed in to change notification settings - Fork 11
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
Rustc: Create Adapter
early to register lints with rustc
#203
Conversation
assert!(config.register_lints.is_none()); | ||
|
||
config.register_lints = Some(Box::new(|_sess, lint_store| { | ||
// Register lints from lint crates. This is required to have rustc track |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these thread_local
required as a cache because the callbacks are invoked on per-crate basis, but the values they return for each crate are the same?
Maybe the adapter / converter / lints map, and other stuff could be a field on MarkerLintPass
? This would avoid some of the thread locals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Marker caches are often used to prevent translating existing notes again and leaking memory. This is a special case, though. Rustc assumes that all lints are static pointers. It uses the memory address as an ID and also for hashing etc. Marker is therefore responsible for keeping the memory addresses of these lints static. That's what this cache is used for.
The MarkerLintPass
of the rustc driver is only conditionally constructed later. I tried around with storing it in there before, but then that instance also needed to be stored somewhere. This was the nicest solution I could come up with, but there are most likely better ones.
The adapter is probably not really possible, as that one in theory shouldn't know about rustc
(The lint pass of the driver should probably have a different one than the one of the lint crate, this confused me at first xD)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While, I don't think I have quite the familiarity with the codebase to give a comprehensive review. I have given this a look and I don't see any problems.
Thank you for the review, you two! Reviewing PRs is also a great way to learn. bors r=NathanFrasier |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Registering the lints early, makes rustc track the lint level correctly (Assuming that marker is registered as a tool)
Not much more to say, I wrote this thing last night and I was super tired at the end. But it works, that's the important part ^^
Closes: #200