Addressed all feedback to date

3 files changed, 23 insertions, 14 deletions

diff --git a/compiler/rustc_mir/src/transform/coverage/counters.rs b/compiler/rustc_mir/src/transform/coverage/counters.rs
index a3ae3021524..7454ec2f438 100644
--- a/compiler/rustc_mir/src/transform/coverage/counters.rs
+++ b/compiler/rustc_mir/src/transform/coverage/counters.rs
@@ -12,6 +12,16 @@ use rustc_data_structures::graph::WithNumNodes;
 use rustc_index::bit_set::BitSet;
 use rustc_middle::mir::coverage::*;
 
+// When evaluating an expression operand to determine if it references a `Counter` or an
+// `Expression`, the range of counter or expression IDs must be known in order to answer the
+// question: "Does this ID fall inside the range of counters," for example. If "yes," the ID refers
+// to a counter, otherwise the ID refers to an expression.
+//
+// But in situations where the range is not currently known, the only fallback is to assume a
+// specific range limit. `MAX_COUNTER_GUARD` enforces a limit on the number of counters, and
+// therefore a limit on the range of counter IDs.
+pub(crate) const MAX_COUNTER_GUARD: u32 = (u32::MAX / 2) + 1;
+
 /// Manages the counter and expression indexes/IDs to generate `CoverageKind` components for MIR
 /// `Coverage` statements.
 pub(crate) struct CoverageCounters {
@@ -95,6 +105,7 @@ impl CoverageCounters {
     /// Counter IDs start from one and go up.
     fn next_counter(&mut self) -> CounterValueReference {
         assert!(self.next_counter_id < u32::MAX - self.num_expressions);
+        assert!(self.next_counter_id <= MAX_COUNTER_GUARD);
         let next = self.next_counter_id;
         self.next_counter_id += 1;
         CounterValueReference::from(next)
diff --git a/compiler/rustc_mir/src/transform/coverage/query.rs b/compiler/rustc_mir/src/transform/coverage/query.rs
index c17221b78dd..2fbbc9b675a 100644
--- a/compiler/rustc_mir/src/transform/coverage/query.rs
+++ b/compiler/rustc_mir/src/transform/coverage/query.rs
@@ -1,3 +1,5 @@
+use super::counters;
+
 use rustc_middle::mir::coverage::*;
 use rustc_middle::mir::visit::Visitor;
 use rustc_middle::mir::{Coverage, CoverageInfo, Location};
@@ -32,21 +34,16 @@ pub(crate) fn provide(providers: &mut Providers) {
 /// safeguard, with `add_missing_operands` set to `true`, to find any other counter or expression
 /// IDs referenced by expression operands, if not already seen.
 ///
-/// Ideally, every expression operand in the MIR will have a corresponding Counter or Expression,
-/// but since current or future MIR optimizations can theoretically optimize out segments of a
-/// MIR, it may not be possible to guarantee this, so the second pass ensures the `CoverageInfo`
-/// counts include all referenced IDs.
+/// Ideally, each operand ID in a MIR `CoverageKind::Expression` will have a separate MIR `Coverage`
+/// statement for the `Counter` or `Expression` with the referenced ID. but since current or future
+/// MIR optimizations can theoretically optimize out segments of a MIR, it may not be possible to
+/// guarantee this, so the second pass ensures the `CoverageInfo` counts include all referenced IDs.
 struct CoverageVisitor {
     info: CoverageInfo,
     add_missing_operands: bool,
 }
 
 impl CoverageVisitor {
-    // If an expression operand is encountered with an ID outside the range of known counters and
-    // expressions, the only way to determine if the ID is a counter ID or an expression ID is to
-    // assume a maximum possible counter ID value.
-    const MAX_COUNTER_GUARD: u32 = (u32::MAX / 2) + 1;
-
     #[inline(always)]
     fn update_num_counters(&mut self, counter_id: u32) {
         self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1);
@@ -62,7 +59,10 @@ impl CoverageVisitor {
         if operand_id >= self.info.num_counters {
             let operand_as_expression_index = u32::MAX - operand_id;
             if operand_as_expression_index >= self.info.num_expressions {
-                if operand_id <= Self::MAX_COUNTER_GUARD {
+                if operand_id <= counters::MAX_COUNTER_GUARD {
+                    // Since the complete range of counter and expression IDs is not known here, the
+                    // only way to determine if the ID is a counter ID or an expression ID is to
+                    // assume a maximum possible counter ID value.
                     self.update_num_counters(operand_id)
                 } else {
                     self.update_num_expressions(operand_id)
diff --git a/compiler/rustc_mir/src/transform/coverage/spans.rs b/compiler/rustc_mir/src/transform/coverage/spans.rs
index 61b5b148053..8549ac0e4af 100644
--- a/compiler/rustc_mir/src/transform/coverage/spans.rs
+++ b/compiler/rustc_mir/src/transform/coverage/spans.rs
@@ -388,8 +388,7 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
         bcb_data
             .basic_blocks
             .iter()
-            .map(|bbref| {
-                let bb = *bbref;
+            .flat_map(|&bb| {
                 let data = &self.mir_body[bb];
                 data.statements
                     .iter()
@@ -404,7 +403,6 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
                             .map(|span| CoverageSpan::for_terminator(span, bcb, bb)),
                     )
             })
-            .flatten()
             .collect()
     }
 
@@ -733,7 +731,7 @@ fn filtered_terminator_span(terminator: &'a Terminator<'tcx>, body_span: Span) -
         // However, in other cases, a visible `CoverageSpan` is not wanted, but the `Goto`
         // block must still be counted (for example, to contribute its count to an `Expression`
         // that reports the execution count for some other block). In these cases, the code region
-        // is set to `None`.
+        // is set to `None`. (See `Instrumentor::is_code_region_redundant()`.)
         TerminatorKind::Goto { .. } => {
             Some(function_source_span(terminator.source_info.span.shrink_to_hi(), body_span))
         }