From 3121ef4fd4cc9e3f23d25e525d5d19e2ae6b8519 Mon Sep 17 00:00:00 2001
From: Kunshan Wang <wks1986@gmail.com>
Date: Sat, 30 Nov 2024 20:04:55 +0800
Subject: [PATCH] More minor changes

Require T in MMTKOption<T> to implemnt FromStr because we use
parse::<T>() anyway.

Move Default out of macro.

Update comments.
---
 src/util/options.rs | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/src/util/options.rs b/src/util/options.rs
index 3a7bed429e..e673d99167 100644
--- a/src/util/options.rs
+++ b/src/util/options.rs
@@ -128,14 +128,14 @@ fn always_valid<T>(_: &T) -> bool {
 /// This type allows us to store some metadata for the option. To get the value of an option,
 /// you can simply dereference it (for example, *options.threads).
 #[derive(Clone)]
-pub struct MMTKOption<T: Debug + Clone> {
+pub struct MMTKOption<T: Debug + Clone + FromStr> {
     /// The actual value for the option
     value: T,
     /// The validator to ensure the value is valid.
     validator: fn(&T) -> bool,
 }
 
-impl<T: Debug + Clone> MMTKOption<T> {
+impl<T: Debug + Clone + FromStr> MMTKOption<T> {
     /// Create a new MMTKOption
     pub fn new(value: T, validator: fn(&T) -> bool) -> Self {
         // FIXME: We should enable the following check to make sure the initial value is valid.
@@ -164,7 +164,7 @@ impl<T: Debug + Clone> MMTKOption<T> {
 }
 
 // Dereference an option to get its value.
-impl<T: Debug + Clone> std::ops::Deref for MMTKOption<T> {
+impl<T: Debug + Clone + FromStr> std::ops::Deref for MMTKOption<T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
@@ -177,7 +177,14 @@ macro_rules! options {
         options!($(#[$outer])*$($name: $type [$validator] = $default),*);
     ];
     ($($(#[$outer:meta])*$name:ident: $type:ty [$validator:expr] = $default:expr),*) => [
-        /// MMTk command line options.
+        /// Options for an MMTk instance.  It affects many aspects of the behavior of the MMTk
+        /// instance, including the number of GC worker threads, the GC plan to use, etc.
+        ///
+        /// Options are set by the VM binding before creating an instance of MMTk.  The VM binding
+        /// usually parses command line options, environment variables, configuration files, etc.,
+        /// to determine the options.  MMTk also provides the [`Options::read_env_var_settings`]
+        /// method which reads environment variables of the form `MMTK_*` and set options.  It can
+        /// be convenient in the early development stage of a VM binding.
         #[derive(Clone)]
         pub struct Options {
             $($(#[$outer])*pub $name: MMTKOption<$type>),*
@@ -209,16 +216,16 @@ macro_rules! options {
                 }
             }
         }
-
-        impl Default for Options {
-            /// By default, `Options` instance is created with built-in default settings.
-            fn default() -> Self {
-                Self::new()
-            }
-        }
     ]
 }
 
+impl Default for Options {
+    /// By default, `Options` instance is created with built-in default settings.
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl Options {
     /// Set an option by name and value as strings.  Returns true if the option is successfully set;
     /// false otherwise.
@@ -228,13 +235,12 @@ impl Options {
     ///
     /// ```rust
     /// let mut builder = MMTKBuilder::new();
-    ///
-    /// // Set values directly
     /// builder.options.threads.set(4);
     /// builder.options.plan.set(PlanSelector::GenImmix);
     ///
-    /// // or convert strings to values.
-    /// builder.options.stress_factor.set(user_input.parse());
+    /// // All `T` in `MMTKOption<T>` implement `FromStr`.
+    /// builder.options.plan.set(user_input1.parse()?);
+    /// builder.options.thread_affinity.set(user_input2.parse()?);
     /// ```
     ///
     /// Only use this method if the option name is also provided as strings, e.g. from command line
@@ -780,8 +786,6 @@ mod gc_trigger_tests {
     }
 }
 
-// Currently we allow all the options to be set by env var for the sake of convenience.
-// At some point, we may disallow this and all the options can only be set by command line.
 options! {
     /// The GC plan to use.
     plan:                   PlanSelector            [always_valid] = PlanSelector::GenImmix,