414 Argument struct for `lucetc::Compiler::new()` by michael-groble · Pull Request #418 · bytecodealliance/lucet

michael-groble · 2020-02-09T15:15:52Z

Approach for #414

First time working in this code base and not a huge amount of rust experience so comments much appreciated!

This is an approach that attempts to make construction simpler without changing the signature for Compiler::new. CompilerBuilder could easily be something more like CompilerOptions that gets passed in as an argument to either a new Compiler method or Compiler::new if that is really the desire.

There are many more places using Compiler::new that I can update once I get some agreement on approach. Also can add some testing. Just throwing something out there fore feedback first.

michael-groble · 2020-02-09T15:21:14Z

lucetc/src/compiler.rs

 }

+pub struct CompilerBuilder {
+    pub target: Triple,


I built this struct before seeing the with_ conventions in LucetOpts. There is one place (Lucetc.shared_object_file) that wants access to this. Making it pub was just a quick way to get something out for review. Can adopt a different naming convention here to differentiate "accessors" vs. "setters".

How about using pub(crate) instead of pub? I don't think exposing this to Lucetc.shared_object_file, but that would keep target from being exposed outside of the crate as well.

You already noticed the with_ conventions, following these here would be nice 🙂 I'll tag one of the methods below with an example of the signature change I would hope to see.

cratelyn · 2020-02-11T18:04:12Z

lucet-wasi-sdk/tests/lucetc.rs

+        let mut builder = Compiler::builder();
+        let c = builder
+            .validator(&Some(v))
+            .create(&m, &b)
+            .expect("compile empty");


This looks much nicer now 🎉

cratelyn

First off, thank you for opening a PR! We appreciate it 🙂

This is a wonderful job of implementing something using the builder pattern in Rust, especially for someone new to Rust, so I also would like to commend you for that! I have a few very small nitpicks regarding method signatures for CompilerBuilder, but this should be ready to merge after a few tiny fixes 🎉

cratelyn · 2020-02-11T18:07:43Z

lucetc/src/compiler.rs

+    pub fn validator(&mut self, validator: &Option<Validator>) -> &mut Self {
+        self.validator = validator.clone();


Do you think that this could accept Option<Validator> instead? This will just move the .clone()'s to the places that this method is called.

cratelyn · 2020-02-11T18:16:36Z

lucetc/src/lib.rs

-            opt_level: OptLevel::default(),
-            cpu_features: CpuFeatures::default(),
-            heap: HeapSettings::default(),
+            builder: Compiler::builder(),


This too, looks much nicer now. 😸

cratelyn · 2020-02-11T18:28:51Z

lucetc/src/compiler.rs

 }

+pub struct CompilerBuilder {
+    pub target: Triple,


How about using pub(crate) instead of pub? I don't think exposing this to Lucetc.shared_object_file, but that would keep target from being exposed outside of the crate as well.

You already noticed the with_ conventions, following these here would be nice 🙂 I'll tag one of the methods below with an example of the signature change I would hope to see.

cratelyn · 2020-02-11T18:30:47Z

lucetc/src/compiler.rs

+        }
+    }
+
+    pub fn target(&mut self, target: Triple) -> &mut Self {


As you mentioned above, this could follow the LucetcOpts conventions 🙂

Instead of having this borrow the struct, we can have it consume self, and return an owned type, via the signature below. This just means passing ownership of the object along, instead of passing along the ability to mutate the object. Call-sites shouldn't change too much. 🤞

pub fn with_target(self, target: Triple) -> Self

if I try this signature, then within LucetOpts impl, it complains that CompilerBuilder needs to implement Copy. Lots of bits in there don't implement Copy (Triple, CpuFeatures, HeapSettings) so I'm wondering if I'm missing something about how to do this. I see how the impl of LucetOpts uses the following signature

fn with_target(mut self, target: Triple) -> Self

but not sure why it doesn't need the Copy. Can dig more.

ok, i think i have this how you're asking @katie-martin-fastly. Note there is still the _mut versions as below. as before, there are a lot more places to clean up if this looks good.

cratelyn · 2020-02-11T18:41:23Z

lucetc/src/compiler.rs

+    pub fn cpu_features_mut(&mut self) -> &mut CpuFeatures {
+        &mut self.cpu_features
+    }


Building off of the with_ changes, I would change the signature of these mutator methods to look like

cpu_features(&mut self, cpu_features: CpuFeatures)

This means code that changes the value of a member, CpuFeatures in this case, does so by passing the new value to our builder rather than borrowing and mutating the existing value.

so if you look at the example of how we expose heap_settings to enable min_reserved_size, LucetOpts wants to be able to manipulate parts of heap_settings, not wholly overwrite it, which is what a signature like that would provide. Alternately, I could not expose heap_settings at all (like LucetOpts doesn't) and instead expose each of the individual modifiable parts (although that is opposite of the pattern it uses with cpu_features). I was trying to find a consistent way to handle both cpu_features and heap_settings where they don't seem to be handled consistently in LucetOpts. Open to this change, but then looking for suggestion on heap_settings

Ah! Thank you for noticing this, it is very thoughtful.

I think the way you have this currently is great! Sometimes exceptions are needed, so an additional cpu_features_mut method because of implementation details seems perfectly reasonable to me. The min_reserved_size methods feel more like concerns of the public API, so I don't think this struct needs to mirror that.

I commented elsewhere about this, but the target_ref method is another case like this. I think it's fine to have these, and the rest of the methods are there to provide a nice API surface for general-use.

cratelyn · 2020-02-11T18:42:33Z

lucetc/src/lib.rs

+    fn canonicalize_nans(&mut self, canonicalize_nans: bool) {
+        self.as_lucetc()
+            .builder
+            .canonicalize_nans(canonicalize_nans);
+    }
+
+    fn with_canonicalize_nans(mut self, canonicalize_nans: bool) -> Self {
+        self.canonicalize_nans(canonicalize_nans);
+        self
+    }


Wonderful touch adding these as well ❤️ Thank you!

Heads up that you will see a merge conflict here¹, now that #423 has landed.

1: https://github.com/bytecodealliance/lucet/pull/423/files#diff-a5ee6b9e6496674851a8b1023f579e5eR261

cratelyn

Sorry about letting this sit for a little while. This is excellent work! I am happy to see how this encapsulates around the call to Compiler::new 🎉

there are a lot more places to clean up if this looks good.

Do you mean that there is more you'd like to do here? If you are satisfied, I think this is already a great step in the right direction. ✔️

cratelyn · 2020-02-22T22:21:03Z

lucetc/src/compiler.rs

+    pub fn cpu_features_mut(&mut self) -> &mut CpuFeatures {
+        &mut self.cpu_features
+    }


Ah! Thank you for noticing this, it is very thoughtful.

I think the way you have this currently is great! Sometimes exceptions are needed, so an additional cpu_features_mut method because of implementation details seems perfectly reasonable to me. The min_reserved_size methods feel more like concerns of the public API, so I don't think this struct needs to mirror that.

cratelyn · 2020-02-22T22:28:22Z

lucetc/src/lib.rs

        let objpath = dir.path().join("tmp.o");
        self.object_file(objpath.clone())?;
-        link_so(objpath, &self.target, &output)?;
+        link_so(objpath, self.builder.target_ref(), &output)?;


You mentioned a similar thing with heap_settings_mut, I think the same thing applies here. As well. Having an extra method doesn't hurt us, especially if it's scoped with pub(crate)

cratelyn · 2020-02-22T22:29:45Z

lucetc/src/compiler.rs

+    pub fn cpu_features_mut(&mut self) -> &mut CpuFeatures {
+        &mut self.cpu_features
+    }


I commented elsewhere about this, but the target_ref method is another case like this. I think it's fine to have these, and the rest of the methods are there to provide a nice API surface for general-use.

michael-groble · 2020-02-29T14:36:00Z

Do you mean that there is more you'd like to do here? If you are satisfied, I think this is already a great step in the right direction.

I just meant the last commit I just added where I clean up uses of Compiler::new in the tests, mostly in lucetc/test/wasm.rs. With that, I think this is complete. Really appreciate the guidance on this @katie-martin-fastly!

cratelyn

🎉 ✔️

…ation

michael-groble commented Feb 9, 2020

View reviewed changes

cratelyn reviewed Feb 11, 2020

View reviewed changes

cratelyn suggested changes Feb 11, 2020

View reviewed changes

michael-groble force-pushed the 414-argument-struct-for-lucetc-compiler-new branch 2 times, most recently from 7e9b7c7 to 9c63f8a Compare February 15, 2020 13:36

iximeow mentioned this pull request Feb 19, 2020

Pinned heap test #273

Closed

michael-groble requested a review from cratelyn February 22, 2020 13:42

cratelyn previously approved these changes Feb 22, 2020

View reviewed changes

michael-groble dismissed cratelyn’s stale review via 10913cf February 29, 2020 14:30

cratelyn previously approved these changes Mar 2, 2020

View reviewed changes

michael-groble added 7 commits March 2, 2020 18:52

create an initial version of CompilerBuilder to simplify compiler cre…

37deab3

…ation

incorporate CompilerBuilder into LucetcOpts

1664156

add convenience method to reduce imports

a6925ef

use with_ naming conventions

53481ca

make target accessible via function

38a66e4

with owned types

14dc1b3

use builder to simplify test setup

f4bfe3b

michael-groble dismissed cratelyn’s stale review via f4bfe3b March 3, 2020 00:57

michael-groble force-pushed the 414-argument-struct-for-lucetc-compiler-new branch from 10913cf to f4bfe3b Compare March 3, 2020 00:57

Merge branch 'master' into 414-argument-struct-for-lucetc-compiler-new

f495bb8

cratelyn approved these changes Mar 19, 2020

View reviewed changes

cratelyn merged commit c765316 into bytecodealliance:master Mar 19, 2020

pchickey mentioned this pull request Apr 3, 2020

Argument struct for lucetc::Compiler::new() #414

Closed

		pub fn validator(&mut self, validator: &Option<Validator>) -> &mut Self {
		self.validator = validator.clone();

Conversation

michael-groble commented Feb 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cratelyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-groble Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cratelyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-groble commented Feb 29, 2020

Uh oh!

cratelyn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michael-groble Feb 15, 2020 •

edited

Loading