Skip to content

Refactor I/O functions for cross-platform support and improve memory …#1

Open
hsiang086 wants to merge 1 commit intoAlex-Velez:mainfrom
hsiang086:main
Open

Refactor I/O functions for cross-platform support and improve memory …#1
hsiang086 wants to merge 1 commit intoAlex-Velez:mainfrom
hsiang086:main

Conversation

@hsiang086
Copy link

This PR introduces cross-platform support for the I/O functions and refactors memory operations for better portability and maintainability. The changes include:

  • Cross-Platform I/O Handling:

    • Added conditional compilation using #[cfg] attributes to separate Windows-specific implementations of _getch and _putch.
    • Implemented alternative getch and putch functions for non-Windows platforms using standard Rust libraries (io::stdin and print).
    • Ensured all I/O operations work consistently across different platforms, removing the dependency on platform-specific C functions for Unix-based systems.
  • Memory Handling Improvements:

    • Adjusted memory operations such as shift_right, shift_left, increment, and decrement to work seamlessly with the new I/O implementations.
    • Refined the adjust_size function to dynamically handle memory expansion based on pointer movements.
  • Code Maintenance and Clarity:

    • Improved code readability and maintainability by using conditional compilation to isolate platform-specific code.
    • Added detailed comments to document the purpose of each function and explain platform-specific behavior.

How to Test

  1. Windows:

    • Verify that the _getch and _putch functions work as expected.
    • Test input, output, and pointer manipulation functions to ensure correct behavior.
  2. macOS / Linux:

    • Confirm that getch reads input correctly using io::stdin.
    • Validate that putch outputs the correct characters and that output flushes stdout properly.

Additional Notes

This refactor ensures that the codebase is more robust and maintainable across different platforms, reducing the likelihood of platform-specific bugs and improving overall compatibility.

// Alternative implementation for non-Windows platforms
#[cfg(not(target_os = "windows"))]
#[inline]
fn getch() -> core::ffi::c_char {
Copy link
Owner

Choose a reason for hiding this comment

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

Although this getch function uses the stand rust library, which is nice to not depend on external C functions, it has a different functionality from the original implementation. The intended implementation for getch should terminate once a single byte has been read, but your implementation allows for multiple characters to be typed in the terminal and requires a \n character or enter key to be pressed before the first byte is read.

Ex: This code should terminate and only show a single character in the terminal after 1 key press.

,.

#[inline]
fn putch(c_char: core::ffi::c_char) {
print!("{}", c_char as u8 as char);
io::stdout().flush().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Flushing is already handled in memory::output(). This is to reduce the amount of times flush() is called when printing multiple times at once.

@Alex-Velez
Copy link
Owner

This PR doesn't seem to change much other than the getch and putch implementations, so I'm not sure why you included the Memory Handling Improvements section, when shift_right, shift_left, increment, decrement, and adjust_size have not been changed.

I appreciate the better comments, but they aren't as detailed since they don't describe everything that happening in the context of the function call. Admittedly, the entire code base should be properly documented, since the current comments are just artifacts of when I wrote these functions, and I never got the chance to fix them.

The How to Test section is confusing since it doesn't really give any instructions for testing. It would be much better if you gave code examples with expected output.

As commented before, the intended implementation for getch should terminate once a single byte has been read. However, this implementation allows for multiple characters to be typed in the terminal and requires the enter key to be pressed before the first byte is read into memory.

This code should terminate immediately after 1 key press and only show a single character in the terminal.

,.

@hsiang086
Copy link
Author

hsiang086 commented Oct 10, 2024

1. Clarification on getch Implementation:

I understand your concern regarding the behavior of the getch function on non-Windows platforms. The reason for the modification was to address a linking error encountered during compilation on macOS. Specifically, the error:

error: linking with cc failed: exit status: 1

This error was caused by the use of _getch and _putch, which are not available in Unix-based systems. Therefore, I implemented an alternative getch function using Rust's standard library to provide cross-platform compatibility.

I acknowledge that this implementation deviates from the expected behavior of getch by requiring an Enter key press. This happens because stdin().read() in Rust's standard library reads input only after a newline character is entered. This isn't ideal for mimicking the behavior of getch, and I will look into using a library like crossterm or termion to replicate the single-character read functionality across platforms without relying on C functions.

2. Flushing in putch Function:

I see your point about the redundant call to flush() in putch. Since flush() is already called in memory::output, I will remove the flush call from the putch function to avoid unnecessary I/O operations. This will ensure that flushing occurs only once, minimizing performance overhead.

3. Memory Handling Improvements:

You're right—there weren't any direct changes to memory handling functions like shift_right, shift_left, increment, decrement, and adjust_size in this PR. I included the "Memory Handling Improvements" section because I reviewed these functions to ensure they worked seamlessly with the modified I/O functions. I now realize that including this section might have been misleading since there were no actual modifications made to those functions. I'll update the PR description to reflect that.

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

Successfully merging this pull request may close these issues.

2 participants