Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pub keyword instead of trivial getters and setters #44

Open
rikhuijzer opened this issue Jan 13, 2025 · 3 comments
Open

Use pub keyword instead of trivial getters and setters #44

rikhuijzer opened this issue Jan 13, 2025 · 3 comments

Comments

@rikhuijzer
Copy link
Owner

I was unsure about whether using the pub keyword on struct fields, like for example,

struct Something {
    pub x: String
}

was a good idea or not. So to be on the safe-side, I decided to keep the fields private and use trivial getter and setter methods instead:

struct Something {
    pub x: String
}
impl Something {
    fn x(&self) -> String {
        self.x.clone()
    }
    fn set_x(&mut self, x: &str) {
        self.x = x;
    }
}

But I recently saw that Wasmtime makes some fields available via the public API. So if it is good enough for Wasmtime it's good enough for me. Let's rewrite all the trivial getters and setters in the library to pub fields.

@rikhuijzer rikhuijzer added the good first issue Good for newcomers label Jan 13, 2025
@rikhuijzer
Copy link
Owner Author

The way to solve this would be to

  1. Fork the xrcf repository
  2. Open the codebase in an editor (preferably with rust language support built in, but it's not necessary)
  3. Find a location in the codebase with trivial getters and setters
  4. Make the field pub and remove the getters and setters
  5. Run the code and look at the compiler errors
  6. Fix all errors and make sure that cargo watch -x test succeeds again
  7. (optional) find more places to remove getters and setters
  8. Make pull request

@rikhuijzer rikhuijzer removed the good first issue Good for newcomers label Jan 20, 2025
Repository owner deleted a comment from codemechanica Jan 20, 2025
rikhuijzer added a commit that referenced this issue Jan 20, 2025
Removes one `Shared` (`Arc`) to simplify the API and replaces trivial
getters and setters by `pub` (#44).
```diff
  pub struct Block {
      pub label: BlockName,
      arguments: Values,
-     ops: Shared<Vec<Shared<dyn Op>>>,
+     pub ops: Vec<Shared<dyn Op>>,
      parent: Option<Shared<Region>>,
  }
```
@SeanPMulligan
Copy link
Contributor

Hi @rikhuijzer I opened a small PR to address one of the getter/setter changes mentioned in this issue. I'm new to Rust but let me know if this PR is helpful and I can dive into the rest of the getter/setter updates.

@rikhuijzer
Copy link
Owner Author

Hi @rikhuijzer I opened a small PR to address one of the getter/setter changes mentioned in this issue. I'm new to Rust but let me know if this PR is helpful and I can dive into the rest of the getter/setter updates.

Yes thank you for the PR. I'm currently having some other priorities but happily merge PRs! Thank you for taking a look

rikhuijzer pushed a commit that referenced this issue Feb 14, 2025
This commit removes the getter and setter methods for
print_ir_before_all and makes the print_ir_before_all field public. It
also removes the setter method for write and makes the writer field
public. Reference to issue #44
SeanPMulligan added a commit to SeanPMulligan/xrcf that referenced this issue Feb 17, 2025
This commit removes the value getter/setter methods and makes the value
field public. It also updates all previous usage of the getter/setter.
References issue rikhuijzer#44
rikhuijzer pushed a commit that referenced this issue Feb 18, 2025
This commit removes the value getter/setter methods and makes the value
field public. It also updates all previous usage of the getter/setter.
References issue #44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants