Skip to content

Conversation

@plotnick
Copy link
Contributor

The source file sled-agent/src/opte/illumos/firewall_rules.rs introduced as part of #1636 had some clippy warnings, but they are only reported on illumos because that file is conditionally compiled.

@plotnick plotnick requested a review from davepacheco October 21, 2022 23:15
vni: &Vni,
mac: &MacAddr6,
) -> Vec<FirewallRule> {
#[allow(clippy::map_flatten)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Justification: the structure of this whole loop is that we accumulate a doubly nested vector of translated rules (one for each host and protocol) and then flatten it twice to get a single vector that covers everything. If we wrote the flattening as .flat_map(...).flatten() as suggested by clippy, it would totally obscure the structure of the loop. It seems correct to replace one level of .map(...).flatten() with .flat_map(...), but (at least in this case) not two.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable. I'd consider a comment explaining that (even if it's just "Clippy doesn't like .map(...).flatten() below, but in this case it's clearer than the alternative."). Up to you.

vni: &Vni,
mac: &MacAddr6,
) -> Vec<FirewallRule> {
#[allow(clippy::map_flatten)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable. I'd consider a comment explaining that (even if it's just "Clippy doesn't like .map(...).flatten() below, but in this case it's clearer than the alternative."). Up to you.

@plotnick plotnick merged commit 6f56748 into main Oct 24, 2022
@plotnick plotnick deleted the firewall-clippy branch October 24, 2022 18:40
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