perf: bail early in too_many_lines lint if rule is not enabled.

[connorshea]

If the rule isn't enabled (which is the default behavior), there's no reason to scan the lines in the given function and count them. This avoids extra work for most users.

changelog: none

[rustbot]

Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews.

In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews.

[hashcatHitman]

community review:

Looks good to me. Similar to the approach already used in infinite_loop.

View changes since this review

[CommanderStorm]

🤔 Would this not be better to do outside of this fn and for all of the variants?
Why only too_many_lines and

So in

rust-clippy/clippy_lints/src/functions/mod.rs

Lines 539 to 565 in cc7fb2d

	impl<'tcx> LateLintPass<'tcx> for Functions {
	fn check_fn(
	&mut self,
	cx: &LateContext<'tcx>,
	kind: intravisit::FnKind<'tcx>,
	decl: &'tcx hir::FnDecl<'_>,
	body: &'tcx hir::Body<'_>,
	span: Span,
	def_id: LocalDefId,
	) {
	let hir_id = cx.tcx.local_def_id_to_hir_id(def_id);
	too_many_arguments::check_fn(cx, kind, decl, hir_id, def_id, self.too_many_arguments_threshold);
	too_many_lines::check_fn(cx, kind, body, span, def_id, self.too_many_lines_threshold);
	not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, def_id);
	misnamed_getters::check_fn(cx, kind, decl, body, span);
	impl_trait_in_params::check_fn(cx, &kind, body, hir_id);
	ref_option::check_fn(
	cx,
	kind,
	decl,
	span,
	hir_id,
	def_id,
	body,
	self.avoid_breaking_exported_api,
	);
	}

Is the check for the others more expensive than the actual lint or why is this done this way? 🤔

View changes since this review

[DanielEScherzer]

community review: actual changes look good to me, @CommanderStorm raises a good point about doing this more generally but that shouldn't block the improvement here

View changes since this review

[rustbot]

r? @Jarcho

rustbot has assigned @Jarcho for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 9 candidates
• 9 candidates expanded to 9 candidates
• Random selection from Jarcho, Manishearth, blyxyas, dswij, llogiq

[connorshea]

🤔 Would this not be better to do outside of this fn and for all of the variants? Why only too_many_lines and

So in

rust-clippy/clippy_lints/src/functions/mod.rs

Lines 539 to 565 in cc7fb2d

	impl<'tcx> LateLintPass<'tcx> for Functions {
	fn check_fn(
	&mut self,
	cx: &LateContext<'tcx>,
	kind: intravisit::FnKind<'tcx>,
	decl: &'tcx hir::FnDecl<'_>,
	body: &'tcx hir::Body<'_>,
	span: Span,
	def_id: LocalDefId,
	) {
	let hir_id = cx.tcx.local_def_id_to_hir_id(def_id);
	too_many_arguments::check_fn(cx, kind, decl, hir_id, def_id, self.too_many_arguments_threshold);
	too_many_lines::check_fn(cx, kind, body, span, def_id, self.too_many_lines_threshold);
	not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, def_id);
	misnamed_getters::check_fn(cx, kind, decl, body, span);
	impl_trait_in_params::check_fn(cx, &kind, body, hir_id);
	ref_option::check_fn(
	cx,
	kind,
	decl,
	span,
	hir_id,
	def_id,
	body,
	self.avoid_breaking_exported_api,
	);
	}

Is the check for the others more expensive than the actual lint or why is this done this way? 🤔

this might be a good idea but I'm not sure if that'd break the ability to enable specific rules mid-file, which I assume is why it works this way right now

[ada4a]

afaik not running a lint on a HIR node if the latter has an allow is an optimization that's already done automatically, so this shouldn't be needed? Or were you able to find some cases which do show perf wins from this?