Add attempt for better module configuration #161

axic · 2019-09-19T22:08:26Z

Closes #135.

axic · 2019-09-19T22:16:17Z

libchisel/src/checkstartfunc.rs

+        Err(ModuleError::NotSupported)
+    }
+
+    fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {


Could also split this into two (and that would remove the need for default):

"with config string"

"with complex config"

e.g.

fn with_config_string(config: &str) fn with_config_map(config: &HashMap<String, String>)

and then with_config_string(&"") would mean "defaults"

I think key-val configuration should be fine for now. No need to further complicate things with the config string, and we can definitely just implement the config string as a specified key config or something in the hashmap.

True, but the downside is the terrible syntax needed for creating a hashmap and passing it. So for simple modules a string is much more simple.

Perhaps the config string method could just wrap that logic?

What do you mean by wrap? Define a way to encode hashmaps?

with_config_string would just call with_config where a dedicated key such as config_string is set to the value provided.

libchisel/src/checkstartfunc.rs

libchisel/src/binaryenopt.rs

libchisel/src/deployer.rs

jakelang · 2019-09-25T18:59:28Z

libchisel/src/dropsection.rs

@@ -29,6 +32,34 @@ impl<'a> ChiselModule<'a> for DropSection {
    }
 }

+impl From<std::num::ParseIntError> for ModuleError {


Could make this more generic, and support all kinds of errors:

impl From<E: Error> for ModuleError { fn from(error: E) -> Self { ModuleError::Custom(error.description().to_string()) } }

True, though I was considering here to wrap it with some text or use NotSupported.

Apparently this is unstable: rust-lang/rust#52662

You could have this blanket impl and then have specialized impls for specific errors

axic · 2019-09-25T19:00:11Z

libchisel/src/dropsection.rs

+                    Ok(DropSection::CustomSectionByIndex(str::parse::<usize>(val)?))
+                }
+                "unknown_by_index" => Ok(DropSection::UnknownSectionByIndex(str::parse::<usize>(
+                    val,


rustfmt is rather weird sometimes

jakelang · 2019-09-25T19:00:25Z

libchisel/src/dropsection.rs

+    }
+
+    fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
+        for (key, val) in config.iter() {


Why do we need the loop here if it will always return on first iteration?

Legacy. I guess it could pick a random key/val and do it from there.

That would result in some strange behavior, I feel.
Perhaps enforcing mutual exclusion of dropsection modes here would make the best sense.

Yeah I'm torn on that one. Enforcing that will add a ton of complexity – that's how I started first.

libchisel/src/remapimports.rs

jakelang · 2019-09-25T19:12:23Z

libchisel/src/lib.rs

@@ -80,6 +81,17 @@ pub trait ModulePreset {
        Self: std::marker::Sized;
 }

+// TODO: move this to be part of ChiselModule


I'm undecided as to whether that is a good idea.

jakelang

Looks good. We should also introduce a PR to remove the ChiselModule trait in the future.

Co-authored-by: Jake Lang <[email protected]>

axic mentioned this pull request Sep 19, 2019

THE BIG CLI REWRITE #94

Closed

axic commented Sep 19, 2019

View reviewed changes

jakelang reviewed Sep 20, 2019

View reviewed changes

libchisel/src/checkstartfunc.rs Show resolved Hide resolved

axic force-pushed the module-config branch 3 times, most recently from 93544f2 to 7bf94eb Compare September 25, 2019 17:42

axic mentioned this pull request Sep 25, 2019

Change ModulePreset to return ModuleError #171

Merged

axic force-pushed the module-config branch 3 times, most recently from 1d88cfd to 6719d0c Compare September 25, 2019 18:54