Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

[kernel] E: Improve readability of build.rs #460

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

LucasGdosR
Copy link
Member

This splits the main function according to nanvix/nanvix#385

Most of it was copy-paste. Some points stand out:

  • setup_toolchain() used &str. I could convert it to String to return it, but I figured using lifetime annotations would be better to avoid allocation and keep the code unchanged.
  • compile_source() could take references & to asm_sources and cflags, but I moved ownership since they're used only once. I passed a reference to out_dir, since it gets used by the other functions.
  • I removed the variable cc, which was used only once. Instead, "gcc".to_string() is called directly in compile_source(). If it were allocated inside setup_toolchain() and returned, it would add yet another argument to compile_source(), or a tuple packing cc and cflags, which would be less readable IMO.

@ppenna ppenna self-requested a review November 7, 2024 15:20
@ppenna ppenna self-assigned this Nov 7, 2024
@ppenna ppenna added the enhancement Enhancement Request on an Existing Feature label Nov 7, 2024
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ppenna ppenna left a comment

Choose a reason for hiding this comment

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

Please add a rustdoc comment to each function in this file. This will help other newcomers to understand what is going on.

Also, could you add the with the following sections to the rustdoc comment:

  • Description
  • Parameters
  • Return Value

This will ensure consistency with other source files in the project.

.

@LucasGdosR
Copy link
Member Author

I have added the rustdoc comment to each function. I also included a struct along with its rustdoc. With this struct, gcc is specified inside the setup_toolchain() function, where it belongs.

I also added a struct to initiallize everything regarding the compiler in the `setup_toolchain()` function, instead of having the compiler program in the middle of the `compile_source()` function. It's more in line with the original code.
@ppenna ppenna force-pushed the enhancement-kernel-build-readability branch from 50e894d to af7596e Compare November 18, 2024 22:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[build] Improve Readability of build.rs
2 participants