Skip to content

Commit

Permalink
Update comment on variable expansion
Browse files Browse the repository at this point in the history
I once thought that `Variable::expand` should require mutable self
because `Quirk::Random` would require mutable access to the random
number generator. However, mutable access to `Variable` depends on
`VariableSet::get_or_new`, which may bring on unexpected side effect of
creating a new variable when the variable does not exist.

Now I think that `Quirk::Random` should resort to internal mutability
with `RefCell` instead of requiring mutable access to `Variable`. This
commit updates the comment to reflect this change.
  • Loading branch information
magicant committed Apr 27, 2024
1 parent 7534090 commit 528c57d
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion yash-env/src/variable/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ impl Variable {
self.read_only_location.is_some()
}

// TODO Should require mutable self
/// Returns the value of this variable, applying any quirk.
///
/// If this variable has no [`Quirk`], this function just returns
Expand All @@ -151,6 +150,7 @@ impl Variable {
/// This function requires the location of the parameter expanding this
/// variable, so that `Quirk::LineNumber` can yield the line number of the
/// location.
#[inline]
pub fn expand(&self, location: &Location) -> Expansion {
super::quirk::expand(self, location)
}
Expand Down
4 changes: 2 additions & 2 deletions yash-env/src/variable/quirk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ pub enum Quirk {
/// the location of the parameter expansion. This `Quirk` is lost when an
/// assignment sets a new value to the variable.
LineNumber,
// TODO $RANDOM
// TODO $PATH
// TODO Random(RefCell<RandomState>)
// TODO Path(...)
}

/// Expanded value of a variable
Expand Down

0 comments on commit 528c57d

Please sign in to comment.