moonbitlang / moon

The build system and package manager for MoonBit
https://moonbitlang.github.io/moon/
GNU Affero General Public License v3.0
188 stars 17 forks source link

Don't strip empty lines in main function #442

Closed lynzrand closed 3 weeks ago

lynzrand commented 3 weeks ago

Stripping them will interfere with styling if the compiler prints anything other than error messages

Superceeds #426

peter-jerry-ye-code-review[bot] commented 3 weeks ago

From the provided git diff output, here are three potential issues or improvements that can be suggested:

  1. Redundant filter and is_empty Check:

    • The original code uses filter(|it| !it.is_empty()) to remove empty lines before processing the lines. This can be simplified by using the lines() method directly, which already skips empty lines.
    • Suggested Change: Replace .split('\n').filter(|it| !it.is_empty()) with .lines().
    • Reason: The lines() method is more idiomatic and directly provides an iterator over non-empty lines, making the code cleaner and more efficient.
  2. Shared println! in Both Branches:

    • In the original code, println!("{content}") is shared in both branches of the if-else statement. This duplication can be avoided by moving the println! outside the if-else statement.
    • Suggested Change: Move println!("{content}") outside the if-else statement and wrap the moonutil::render::MooncDiagnostic::render(content, use_fancy); call with an if !output_json condition.
    • Reason: This reduces code duplication and makes the logic clearer.
  3. Locking catcher Mutex:

    • The catcher.lock().unwrap().push(content.to_owned()); line locks the mutex every time a line is processed. This can be optimized by locking the mutex once and then processing all lines within the same locked scope.
    • Suggested Change: Lock the mutex once, process all lines, and then unlock it.
    • Reason: This reduces the overhead of locking and unlocking the mutex for each line, potentially improving performance.

Combining these suggestions, the revised code could look something like this:

let catcher = Arc::clone(&logger);
let output_json = moonbuild_opt.output_json;
let render_and_catch = move |output: &str| {
    let mut locked_catcher = catcher.lock().unwrap();
    output.lines().for_each(|content| {
        locked_catcher.push(content.to_owned());
        if output_json {
            println!("{content}");
        } else {
            moonutil::render::MooncDiagnostic::render(content, use_fancy);
        }
    });
};

This approach ensures the code is more idiomatic, avoids unnecessary duplication, and potentially improves performance by reducing mutex locking overhead.