LemonOS - Corrections.

Kernel/src/Fs/VolumeManager.cpp

@@ -3,6 +3,71 @@
 #include <Fs/FsVolume.h>
 #include <Panic.h>
 
+/*
+
+Observation:
+
+    1. Redundancy between Mount(FsNode*, const char*) and Mount(FsNode*, FsDriver*, const char*)
+    Problem:
+    The first function simply identifies the driver and calls the second. This is valid, but can cause confusion due to name duplication.
+
+    Improvement:
+    Rename the first to MountWithAutoDriver(...) or leave it as is but document that it is a helper.
+
+    Alternatively, unify both functions if the driver can always be identified internally.
+
+    2. MountSystemVolume() has scanning logic that could be modularized.
+
+    Problem:
+    The /dev scan function identifies devices and mounts the first one that works. However, this logic is embedded and not reusable.
+
+    Improvement:
+    Extract the scanning logic into a function like TryMountFromDevice(FsNode*).
+
+    This allows the scan to be reused in other contexts (e.g., recovery, fallback).
+
+    3. Using assert() in production
+
+    "assert(controller->Identify(device));
+    "
+
+    Problem:
+    If Identify() fails, the system aborts. Is this desirable in production?
+
+    Improvement:
+    Replace with an if (!driver->Identify(device)) and return an error if critical.
+
+    Use assert() only if the failure involves internal corruption.
+
+    4. SystemVolume() returns a pointer without actual validation
+
+    "assert(systemVolume);
+    return systemVolume;
+    "
+
+    Problem:
+    If systemVolume is not mounted, the system aborts. This can be dangerous if called before MountSystemVolume().
+
+    Improvement:
+    Return nullptr and let the caller handle the error.
+
+    Or document that SystemVolume() should only be called after initialization.
+
+    5. nextVolumeName and nextVolumeID are globals with no persistence.
+
+    Note:
+    These counters are reset on every boot. If the system supports persistence, they should be saved.
+
+    Improvement:
+    If persistent metadata is supported, save these values ​​to disk.
+
+    If not, leave them as is but document that the IDs are not stable between boots.
+
+*/
+
+
+
+
 namespace fs {
 namespace VolumeManager {
 
@@ -128,4 +193,4 @@ FsVolume* SystemVolume() {
 }
 
 } // namespace VolumeManager
-} // namespace fs
+} // namespace fs

Kernel/src/Logging.cpp

@@ -9,6 +9,94 @@
 #include <Video/VideoConsole.h>
 #include <stdarg.h>
 
+/*
+Note:
+
+    The code has versions of Warning, Error, and Information that receive "unsigned long long num," but they either don't do anything useful or are incomplete. E.g.:
+
+    "
+    void Warning(unsigned long long num) {
+    Write("\r\n[WARN] ", 255, 255, 0);
+    // Write(str);
+    }
+
+    "
+    Possible solution:
+    This function does nothing with num. The comment // Write(str); looks like a placeholder.
+
+    Remove it if it's not used in the project.
+
+    If necessary, implement it correctly using Write(num, hex) as in other functions.
+
+    Second observation:
+
+    "Write" with unused RGB parameters:
+
+    " void Write(const char* str, uint8_t r, uint8_t g, uint8_t b) {
+    WriteN(str, strlen(str));
+    }
+    "
+
+    Problem:
+    The parameters r, g, and b are not used in this function. If the color isn't applied, why pass them?
+
+    Solution:
+    If the color isn't used, remove the parameters.
+
+    If you plan to use them in the future, leave an explanatory comment.
+
+    Third observation:
+
+    3. WriteF has complex logic that could be modularized (?)
+
+    Problem:
+    The WriteF function has a very long and difficult-to-maintain format parser.
+
+    Solution:
+    Extract the parser into a separate function such as ParseFormatToken(...).
+
+    This improves readability and allows the parser to be tested separately.
+
+    Fourth observation:
+
+    4. "Write" for numbers could be unified
+    There are two functions for writing numbers:
+    "
+    void Write(unsigned long long num, bool hex, uint8_t r, uint8_t g, uint8_t b)
+    void Write(unsigned long long num, bool hex)
+    "
+
+    Problem:
+    Duplicated logic. The RGB version doesn't use colors.
+
+    Solution:
+    Unify into a single function.
+
+    If colors are not used, remove the parameters.
+
+    Fifth observation:
+
+    5. "Ioctl" in "LogDevice" always returns 0 or -1
+
+    "
+    int Ioctl(uint64_t cmd, uint64_t arg) {
+    if (cmd == TIOCGWINSZ)
+    return 0; // Intend to be a terminal
+    else
+    return -1;
+    }
+
+    "
+
+    Problem:
+    The function doesn't do anything useful. Is it really needed?
+
+    Solution:
+    If the device doesn't need Ioctl, consider removing it.
+
+    If it's required for compatibility, leave a comment explaining why.
+*/
+
 namespace Log {
 VideoConsole* console = nullptr;
 
@@ -89,7 +177,7 @@ void WriteN(const char* str, size_t n) {
             }
 
             if (n + logBufferPos > logBufferMaxSize || !CheckInterrupts()) {
-                size_t discard = (n + logBufferPos) - logBufferSize; // Amount of bytes to discard
+                size_t discard = (n + logBufferPos) - logBufferSize; // Discarded bytes to maintain buffer size
 
                 logBufferPos -= discard;
                 memcpy(logBuffer, logBuffer + discard, logBufferPos);

Kernel/src/Math.cpp

@@ -1,8 +1,56 @@
-unsigned long int rand_next = 1;
+/* Note:
+
+    1. Global mutable state: rand_next
+    Problem:
+    rand_next is a modifiable global variable with no concurrency protection.
+
+    If this code runs in a multithreaded environment, it may cause inconsistent results or state corruption.
+
+    Solution:
+    If the environment is multithreaded, encapsulate rand_next in a thread-local variable or protect it with a lock.
+
+    If it's single-threaded (as in many kernels), leave it there but document that it's not thread-safe.
+
+    2. Custom rand() implementation
+    Note:
+    This is the classic Linear Congruential Generator (LCG) algorithm used in many systems.
+
+    Although functional, it is weak in randomness and not suitable for cryptography.
+
+    Improvement:
+    If used for non-critical purposes (such as animations, delays, etc.), it's fine.
+
+    If used for security, you should replace it with a stronger RNG.
+
+    3. Custom abs() function
+    Problem:
+    Manually defining int abs(int) is unnecessary if <cstdlib> or <cmath> are available.
+
+    Also, it can cause conflicts if you include a standard library that already defines abs.
+
+    Solution:
+    Remove the function and use std::abs() if you're in an environment with access to the STL.
+
+    If you're in a kernel environment without an STL, then it's fine to have it, but you should rename it to avoid collisions.
+*/
+
+
+unsigned long int rand_next = 1; // <-- It's best to set it as "static unsigned long int" ("unsigned long int" Not thread safe)
 
 unsigned int rand() {
     rand_next = rand_next * 1103515245 + 12345;
     return ((unsigned int)(rand_next / 65536) % 32768);
 }
 
-int abs(int num) { return num < 0 ? -num : num; }
+// If standard libraries are unavailable, keep this.
+// Otherwise, prefer std::abs from <cstdlib> or <cmath>.
+
+int abs(int num) { return num < 0 ? -num : num; }
+
+// solution:
+
+/*
+int kernel_abs(int num) {
+    return num < 0 ? -num : num;
+}
+*/

Kernel/src/TTY/PTY.cpp

@@ -27,6 +27,28 @@ PTYDevice::PTYDevice() {
 
 }
 
+// This pull request improves code clarity and formatting in PTY.cpp.
+// It removes redundant comments and aligns with LemonOS's coding style.
+//
+// Detailed Context:
+// - Redundant comments were removed because they repeated what the code already expressed.
+//   For example, comments like "Buffer must be full so just keep trying" were unnecessary,
+//   as the loop logic is self-explanatory.
+// - Comments that explain non-obvious behavior (e.g. signaling watchers, escape handling)
+//   were retained and rewritten in concise English.
+// - Formatting was adjusted for consistency: spacing, indentation, and block structure
+//   now match LemonOS's kernel style.
+// - No functional changes were made. All logic paths, assertions, and system calls remain intact.
+// - These changes improve readability, reduce cognitive load, and help future contributors
+//   understand the code faster.
+//
+// Motivation:
+// - To align this file with the rest of the LemonOS codebase.
+// - To make future diffs cleaner and easier to review.
+// - To support maintainability and onboarding for new developers.
+
+
+
 ssize_t PTYDevice::Read(size_t offset, size_t size, uint8_t* buffer) {
     assert(pty);
     assert(device == PTYSlaveDevice || device == PTYMasterDevice);
@@ -48,53 +70,45 @@ ssize_t PTYDevice::Write(size_t offset, size_t size, uint8_t* buffer) {
 
     if (pty && device == PTYSlaveDevice) {
         ssize_t written = pty->SlaveWrite((char*)buffer, size);
-
         if (written < 0 || written == size) {
-            return written; // Check either for an error or if all bytes were written
+            return written;
         }
 
-        // Buffer must be full so just keep trying
         buffer += written;
         while (written < size) {
             size_t ret = pty->SlaveWrite((char*)buffer, size - written);
-
             if (ret < 0) {
-                return ret; // Error
+                return ret;
             }
-
             written += ret;
             buffer += ret;
         }
 
         return written;
     } else if (pty && device == PTYMasterDevice) {
         ssize_t written = pty->MasterWrite((char*)buffer, size);
-
         if (written < 0 || written == size) {
-            return written; // Check either for an error or if all bytes were written
+            return written;
         }
 
-        // Buffer must be full so just keep trying
         buffer += written;
         while (written < size) {
             size_t ret = pty->MasterWrite((char*)buffer, size - written);
-
             if (ret < 0) {
-                return ret; // Error
+                return ret;
             }
-
             written += ret;
             buffer += ret;
         }
 
         return written;
-    } else {
-        assert(!"PTYDevice::Write: PTYDevice is designated neither slave nor master");
     }
 
+    assert(!"PTYDevice::Write: PTYDevice is designated neither slave nor master");
     return 0;
 }
 
+
 int PTYDevice::Ioctl(uint64_t cmd, uint64_t arg) {
     assert(pty);
 
@@ -327,4 +341,4 @@ void PTY::WatchSlave(FilesystemWatcher& watcher, int events) {
 
 void PTY::UnwatchMaster(FilesystemWatcher& watcher) { m_watchingMaster.remove(&watcher); }
 
-void PTY::UnwatchSlave(FilesystemWatcher& watcher) { m_watchingSlave.remove(&watcher); }
+void PTY::UnwatchSlave(FilesystemWatcher& watcher) { m_watchingSlave.remove(&watcher); }