Fix documentation and style issues.

This patch just has a bunch of style and doc fixes
that I noticed when going over the diff on github.

Author: Ethan Pailes

src/analysis.rs

@@ -77,7 +77,7 @@ impl IsOnePassVisitor {
         self.0 = self.0 && !fsets_clash(&es.iter().collect::<Vec<_>>());
     }
 
-    // Unicode classes are really big alternatives from the byte
+    // Unicode classes are really just big alternatives from the byte
     // oriented point of view.
     //
     // This function translates a unicode class into the 
@@ -100,7 +100,7 @@ impl IsOnePassVisitor {
                     }
                 }
             }
-            _ => {} // FALLTHROUGH
+            _ => {}
         }
     }
 
@@ -129,14 +129,14 @@ fn fsets_clash(es: &[&Hir]) -> bool {
 
 /// Compute the first set of a given regular expression.
 ///
-/// The first set of a regular expression is the set of all characters
+/// The first set of a regular expression is the set of all bytes
 /// which might begin it. This is a less general version of the
 /// notion of a regular expression preview (the first set can be
 /// thought of as the 1-preview of a regular expression).
 ///
 /// Note that first sets are byte-oriented because the DFA is
 /// byte oriented. This means an expression like /Δ|δ/ is actually not
-/// one-pass, even though there is clearly no non-determinism inherent
+/// onepass, even though there is clearly no non-determinism inherent
 /// to the regex at a unicode code point level (big delta and little
 /// delta start with the same byte).
 fn fset_of(expr: &Hir) -> FirstSet {
@@ -204,7 +204,8 @@ fn fset_of(expr: &Hir) -> FirstSet {
 
         // The most involved case. We need to strip leading empty-looks
         // as well as take the union of the first sets of the first n+1
-        // expressions where n is the number of leading repetitions.
+        // expressions where n is the number of leading expressions which
+        // accept the empty string.
         &HirKind::Concat(ref es) => {
             let mut fset = FirstSet::empty();
             for (i, e) in es.iter().enumerate() {
@@ -223,9 +224,9 @@ fn fset_of(expr: &Hir) -> FirstSet {
                         if !inner_fset.accepts_empty() {
                             // We can stop accumulating after we stop seeing
                             // first sets which contain epsilon.
-                            // Also, a contatination which terminated by
+                            // Also, a concatenation which is terminated by
                             // one or more expressions which do not accept
-                            // epsilon itself does not acceept epsilon.
+                            // epsilon itself does not accept epsilon.
                             fset.accepts_empty = false;
                             break;
                         }
@@ -246,8 +247,8 @@ fn fset_of(expr: &Hir) -> FirstSet {
 
 /// The first byte of a unicode code point.
 ///
-/// We only ever care about the first byte of a particular character,
-/// because the onepass DFA is implemented in the byte space, not the
+/// We only ever care about the first byte of a particular character
+/// because the onepass DFA is implemented in the byte space not the
 /// character space. This means, for example, that a branch between
 /// lowercase delta and uppercase delta is actually non-deterministic.
 fn first_byte(c: char) -> u8 {

src/exec.rs

@@ -449,8 +449,6 @@ impl<'c> RegularExpression for ExecNoSync<'c> {
                         let mut slots = vec![None; self.slots_len()];
                         if op.exec(&mut slots, text, start) {
                             slots[0]
-                            // TODO(ethan): why did I chain?
-                            // slots[1].and_then(|_| slots[0])
                         } else {
                             None
                         }
@@ -513,13 +511,6 @@ impl<'c> RegularExpression for ExecNoSync<'c> {
                     Some(ref op) => {
                         let mut slots = vec![None; self.slots_len()];
                         op.exec(&mut slots, text, start)
-                        /* TODO: why did I check capture slots??
-                        if op.exec(&mut slots, text, start) {
-                            slots[0].is_some() && slots[1].is_some()
-                        } else {
-                            false
-                        }
-                        */
                     }
                     None => unreachable!(),
                 }
@@ -1340,9 +1331,7 @@ enum MatchType {
     /// can be decomposed into unambiguous literal search.
     Literal(MatchLiteralType),
     /// A onepass DFA search. If a onepass search is impossible, we just
-    /// fall back to an `Nfa(MatchNfaType::Auto)` search. The execution
-    /// planner will never select the `OnePassDfa` match type when
-    /// there will be a fallback.
+    /// fall back to an automatically chosen search.
     OnePassDfa(Box<Option<MatchType>>),
     /// A normal DFA search.
     Dfa,

src/onepass.rs

@@ -286,11 +286,15 @@ impl OnePass {
                 // No need to mask because no flags are set.
                 state_ptr = self.follow(state_ptr as usize, byte_class);
             } else {
+                // STATE_HALT and STATE_DEAD must always be checked
+                // first because they have STATE_ACTION and STATE_MATCH
+                // set, even though those flags don't apply. It would
+                // probably be better for performance to check them last,
+                // so it may be worthwhile to try to rejigger the
+                // representation of StatePtrs.
                 if state_ptr == STATE_HALT {
                     break;
-                }
-
-                if state_ptr == STATE_DEAD {
+                } else if state_ptr == STATE_DEAD {
                     trace!("::exec_ drain-dead");
                     slots[FULL_MATCH_CAPTURE_END] = last_match;
                     return last_match.is_some();
@@ -474,7 +478,7 @@ pub struct OnePassCompiler {
     /// if they should have the STATE_MATCH flag set.
     accepting_states: Vec<bool>,
 
-    /// A DAG of forwarding relationship indicating when
+    /// A DAG of forwarding relationships indicating when
     /// a state needs to be forwarded to an Action state
     /// once that Action state has been fully constructed.
     forwards: Forwards,
@@ -595,6 +599,13 @@ impl OnePassCompiler {
         Ok(self.onepass)
     }
 
+    /// Compile the stage 1 transition table for the state corresponding
+    /// to the given instruction.
+    ///
+    /// The result of `inst_trans` will end up in `self.transitions`.
+    ///
+    /// Returns a list of child instructions which must be compiled
+    /// via `inst_trans`.
     fn inst_trans(
         &mut self,
         inst_idx: usize
@@ -671,6 +682,45 @@ impl OnePassCompiler {
         Ok(children)
     }
 
+    /// Topologically sort the forwarding jobs so that we
+    /// start with jobs that have no dependencies and then
+    /// shuffle the transitions over. Mutate `self.transitions`
+    /// in place.
+    ///
+    /// To make that a little more concrete, consider the program snippet:
+    ///
+    /// 0000: Bytes(a, a)
+    /// 0001: Save(2)
+    /// 0002: Bytes(b, b)
+    ///
+    /// Here the state for `Bytes(a, a)` needs to transition to
+    /// the state for `Save(2)`, but it does not know when to do
+    /// so. The right answer is that it should transition to
+    /// the `Save(2)` state when it sees a `b`, but it is hard
+    /// to know what children `Save(2)` has from where `Bytes(a, a)`
+    /// stands. To handle this we just emit a forwarding job
+    /// that says "when you know enough about the `Save(2)` state,
+    /// please forward `Bytes(a, a)` to `Save(2)`.". We need to use
+    /// a full DAG for this because there could be multiple forwarding
+    /// states in a row:
+    ///
+    /// 0000: Bytes(a, a)
+    /// 0001: Save(2)
+    /// 0002: Save(3)
+    /// 0003: Bytes(b, b)
+    ///
+    /// Here we will end up with two forwarding jobs:
+    ///
+    /// 1. Forward from `Bytes(a, a)` to `Save(2)`.
+    /// 2. Forward from `Save(2)` to `Save(3)`.
+    ///
+    /// Which we structure as a dag that looks like:
+    ///
+    /// (2) --> (1)
+    ///
+    /// The arrow flows in a funny direction because we want the jobs
+    /// with no dependencies to live at the roots of the DAG so that
+    /// we can process them first.
     fn solve_forwards(&mut self) -> Result<(), OnePassError> {
         // TODO(ethan):yakshaving drop the clone
         for fwd in self.forwards.clone().into_iter_topo() {
@@ -718,9 +768,9 @@ impl OnePassCompiler {
             }
 
             // Finally, if a match instruction is reachable through
-            // a save fwd, the from state is accepting.
-            match (&self.prog[fwd.to], &self.prog[fwd.from]) {
-                (&Inst::Save(_), _) => {
+            // a save fwd (which can never fail), the from state is accepting.
+            match &self.prog[fwd.to] {
+                &Inst::Save(_) => {
                     self.accepting_states[fwd.from] =
                         self.accepting_states[fwd.to];
                 }
@@ -731,11 +781,12 @@ impl OnePassCompiler {
         Ok(())
     }
 
-    // Once all the per-instruction transition tables have been worked
-    // out, we can bake them into the single flat transition table we
-    // are going to use for the actual DFA.
+    /// Once all the per-instruction transition tables have been worked
+    /// out, we can bake them into the single flat transition table we
+    /// are going to use for the actual DFA. This function creates the
+    /// baked form, storing it in `self.onepass.table`.
     fn emit_transitions(&mut self) {
-        // pre-compute the state indicies
+        // pre-compute the state indices
         let mut state_starts = Vec::with_capacity(self.prog.len());
         let mut off = 0;
         for inst_idx in 0..self.prog.len() {
@@ -760,10 +811,12 @@ impl OnePassCompiler {
             p
         };
 
+        self.onepass.table.reserve(
+            state_starts[state_starts.len() - 1]
+            + self.onepass.num_byte_classes);
         for inst_idx in 0..self.prog.len() {
             let mut trans = Vec::with_capacity(
-                state_starts[state_starts.len() - 1]
-                + self.onepass.num_byte_classes);
+                self.onepass.num_byte_classes * 2);
 
             match &self.transitions[inst_idx] {
                 &None => continue,
@@ -1102,7 +1155,7 @@ const STATE_DEAD: StatePtr = STATE_MATCH + 1;
 /// from DEAD only in that an accepting state that transitions to HALT
 /// still accepts, while an accepting state which transitions to DEAD
 /// does not.
-const STATE_HALT: StatePtr = STATE_MATCH + 2;
+const STATE_HALT: StatePtr = STATE_ACTION + 1;
 
 /// The maximum state pointer. This is useful to mask out the "valid" state
 /// pointer from a state with the "start" or "match" bits set.