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

Cleanups #19

Merged
merged 10 commits into from
Jul 3, 2023
Merged

Cleanups #19

merged 10 commits into from
Jul 3, 2023

Conversation

oowekyala
Copy link
Contributor

These are a couple of cleanups:

  • using Result instead of bool to not hide errors.
  • remove a some clones and simplify some things. For instance I removed the CodeGenerator struct because it is not useful to create a struct just to hold some function parameters and then directly call the function. It causes extra clones because the struct contains not references but owned objects.

I moved my cargo-related changes out of this PR for later.


// compiling
let mut cmake_build_command = Command::new("cmake");
cmake_build_command.current_dir(format!("{}/build", self.lfc.out.display().to_string()));
cmake_build_command.arg("--build");
cmake_build_command.arg("./");
let cmake_build = run_and_capture(&mut cmake_build_command).is_ok();
run_and_capture(&mut cmake_build_command)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how you use result: if the function run_and_capture returns an error result, then the question mark operator here causes the current function to return the same error result. That way in the rest of this function you can assume that the call succeeded.

@tanneberger tanneberger added the enhancement New feature or request label Jul 2, 2023
@tanneberger
Copy link
Member

I did some minor clean up and this is now ready to merge.

This was referenced Jul 2, 2023
Copy link
Member

@tanneberger tanneberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me

@erlingrj
Copy link
Collaborator

erlingrj commented Jul 3, 2023

These look like good changes to me! I saw some TODO regarding the purpose of different structs. I think we would benefit from some discussion about how we should structure this program into different structs, traits and their relationship. Would be great with a class diagram or something. I am still a little confused by the App vs AppFile and Config vs ConfigFile.

Also, I am little confused by the to introduce the lifetime specifiers <'a>. It seems it is used to make sure a reference to the struct doesn't outlive references to its members. I would have guessed this to be the default and that you should essentially annotate all your structs with this

@oowekyala
Copy link
Contributor Author

These look like good changes to me! I saw some TODO regarding the purpose of different structs. I think we would benefit from some discussion about how we should structure this program into different structs, traits and their relationship. Would be great with a class diagram or something. I am still a little confused by the App vs AppFile and Config vs ConfigFile.

Ideally I think the structs used for CLI args should not make it past main, and eg not be passed to the backend. I have some local changes where I refactored the backend interface in that direction, but probably we should have a broader discussion.

Also, I am little confused by the to introduce the lifetime specifiers <'a>. It seems it is used to make sure a reference to the struct doesn't outlive references to its members. I would have guessed this to be the default and that you should essentially annotate all your structs with this

You only need lifetime parameters for structs that have reference fields. The lifetime of all references in a struct must be quantified by such a parameter, so that the type checker can know where in the program the contained references are valid. The type checker requires that the contained references outlive the struct instance, it's not just about references to members.

@erlingrj erlingrj merged commit daf705b into main Jul 3, 2023
src/args.rs Show resolved Hide resolved
@oowekyala oowekyala deleted the clem.cleanup branch July 3, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants