Migration of LintDiagnostic - part 5#153152
Migration of LintDiagnostic - part 5#153152GuillaumeGomez wants to merge 6 commits intorust-lang:mainfrom
Conversation
|
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Migration of LintDiagnostic - part 5
| tcx: Option<TyCtxt<'_>>, | ||
| diagnostic: BuiltinLintDiag, | ||
| diag: &mut Diag<'_, ()>, | ||
| lint: &'static Lint, |
There was a problem hiding this comment.
Does this function need to know about lint? Could lint just be inside the ctx instead?
There was a problem hiding this comment.
Sounds good to me!
src/librustdoc/lib.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| struct DiagEmitter<'tcx> { |
There was a problem hiding this comment.
Would it be possible to reuse the DiagEmitter struct from rustc_hir_analysis here instead?
If that's annoying dependency-wise, can we put the diag-emitter struct in a logical shared location that both can access? Bonus points if we manage to move the entire "emit delayed lints" for loop below into a shared lcoation
If not to both then this is ok, just a bit of sad duplication
There was a problem hiding this comment.
No, duplication bad. Me removing duplication. 🔨
There was a problem hiding this comment.
(Yeah ok I should maybe go sleep...)
| decorator.decorate_lint(lint); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be hard to keep this function but make it emit a diagnostic instead?
I think I prefer having it generic over having a specific NonUpperCaseGlobalGenerator struct
There was a problem hiding this comment.
Noting it for a next PR then. :)
| tcx: Option<TyCtxt<'_>>, | ||
| diagnostic: BuiltinLintDiag, | ||
| diag: &mut Diag<'_, ()>, | ||
| lint: &'static Lint, |
There was a problem hiding this comment.
I've added to the TODO list in the tracking issue: See if we can get rid of BuiltinLintDiag in favour of a boxed diagnostic of some kind
There was a problem hiding this comment.
That would be very nice indeed.
|
Simplified |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0c55ed2): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary -6.1%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 481.72s -> 480.975s (-0.15%) |
|
If not too annoying I'd like to see the perf regression fixed as well |
d39a00a to
f3d639f
Compare
Part of #153099.
With this,
rust_lintis finally done, although the change of API ofdecorate_builtin_lintimpacted a few other crates, although minimal, still needed to be mentioned.r? @JonathanBrouwer