Former professional software dev here.

https://www.reddit.com/r/programming/comments/5r0f7e/toaruos_10_a_hobby_operating_system/dd3rlwe/

First of all, good job. I wouldn't know how to build my own OS, or even where to start. I could learn much from you.

Second, I opened up kernel/main.c expecting the code cleanliness to at least meet my expectations, and was disappointed. I don't want to be a downer here, because I think you're exceptionally talented --- and I mean exceptionally --- however, there are a few professional tips you could learn. You don't have to listen to me, of course; however, I learned these long ago and they've been an immense help. Finally, I realize and respect that this is your personal project, but it may not be someday.

I'll work my way down through kernel/main.c. The top comment block is good, but could explain the file's purpose a bit.

uintptr_t initial_esp = 0;

(1) I have no idea what "initial_esp" is or why it is initialized to zero. Is its value a literal for a reason? Should it be INITIAL_ESP_VALUE or something, so I don't have to somehow (it can take days!) find and replace 40 magic zeros if the requirements change?

fs_node_t * ramdisk_mount(uintptr_t, size_t);

(2) I believe I understand what you're doing here, but an affirming comment would be helpful.

#define EARLY_LOG_DEVICE 0x3F8

(3) Good job not repeating 0x3F8 throughout your code.

struct pack_header { ... }

(4) What is this struct for? And is there a reason the size of the head[] array is a literal number 4 instead of a #define?

mboot_mag

(5) What's a "mag"?

ENABLE_EARLY_BOOT_LOG(0);

(6) I suggest changing this to ENABLE_EARLY_BOOT_LOG(LOG_LEVEL_WARN); or something like that.

"Didn't boot with multiboot, not sure how we got here."

(7) It would be cleaner if all of your literal strings were neatly organized in a central location.

/* Initialize core modules */

(8) Nicely organized. Great job.

mboot_ptr->flags & (1 << 3)

(9) Definitely should be broken out into its own function, like flag_something_is_set() for example

debug_print(NOTICE, "There %s %d module%s starting at 0x%x.", mboot_ptr->mods_count == 1 ? "is" : "are", mboot_ptr->mods_count, mboot_ptr->mods_count == 1 ? "" : "s", mboot_ptr->mods_addr);

(10) Inline ternaries really slow down someone who's trying to scan your code quickly. If it's not performance-critical, definitely break them out into helper functions.

(mboot_ptr->mods_count > 0)

(11) Literal zero here is fine, since I'm assuming that it means a count of zero. My preference would be NO_MODULES, but a zero is fine. Also, don't be afraid to actually say "modules" instead of "mod" when you aren't using an interpreted language.

(uintptr_t)mod + sizeof(mboot_mod_t) > last_mod

(12) This could be a lot more clear. How about a function that returns a boolean instead?

if (last_mod < module_end) { ... }

(13) What is happening here? I need to be able to figure this out quicker.

if (mmap->type == 2) { ... }

(14) The literals in this block need clarification: 2, 0x1000, 0xFFFFFFFF, 0xFFFFF000

char cmdline_[1024];

(15) How about DEFAULT_CMDLINE_BUF_SIZE or something?

check_result == 2

(16) What does 2 mean?

void * start = (void *)((uintptr_t)pack_header + 4096);

(17) Again, 4096 could be a descriptive #define

(result != 1)

(18) *Maybe (!result_valid()) instead?"

map_vfs_directory("/dev");

(19) Replacing "/dev" with something like DEVICE_DIRECTORY_NAME (or whatever) would be much safer.

(1) I have no idea what "initial_esp" is or why it is initialized to zero. Is its value a literal for a reason? Should it be INITIAL_ESP_VALUE or something, so I don't have to somehow (it can take days!) find and replace 40 magic zeros if the requirements change?

I'd imagine it's the initial value to be loaded into the ESP register on x86 (which is the 32-bit stack pointer, IIRC). Though depending on whether that variable itself is actually modified later on, they might be better off declaring it as a constant.

(3) Good job not repeating 0x3F8 throughout your code.

Unless they're targeting very old systems, it would probably be better to go with a const than a define.

(10) Inline ternaries really slow down someone who's trying to scan your code quickly. If it's not performance-critical, definitely break them out into helper functions.

Functions add overhead, but appropriate use of whitespace would help make these things more readable.

>>2
yes...

YHBT. 7/10.

Inline ternaries really slow down someone who's trying to scan your code quickly.

Maybe [1] you should not be "trying to scan your code quickly" and instead actually read and understand it, or [2] you're just another one of those idiots who whines about "readability" because YOU are too stupid to understand what the code is doing.

>>4
stop begin a namefag

ciode shouled be hard to read beacues i'm smaret

.c

Great. More buffer overflow garbage.

a professional programmer doesn't know that esp is stack pointer and it's obvious why it should be 0-initialized without the need to #define it? this makes me feel better about my level of knowledge

autism

>>8
"Amateurs built the Ark. Professionals built the Titanic."

(I'm not actually religious. That quote just came to mind.)

Holy Gods he is retarded. I know nothing about C and all of what he complains about is perfectly understandable.

>>10

I'm not religious

*tip*

>>8

esp is stack pointer

You need ESP to figure out what Intel's garbage means.

P = Pointer
S = Stack
E = Extended

What's so hard to figure out?

Functions add overhead, but

🙄

>>13
sure, but if you ever did systems programming on x86 then you already know what it is. and if you never did then don't expect to intuitively understand x86 kernel source code.

(12) This could be a lot more clear. How about a function that returns a boolean instead?

C doesn't have booleans. Fucking Indians trying to give advice on something they don't even understand. I hate Indians so fucking much.

>>17

I wouldn't know how to build my own OS, or even where to start. I could learn much from you

But let me go through the first file I found and criticize it line by line

Extended Stick Pointer in my anus!

>>17

C doesn't have booleans

READ DA STANDARD

Key word here: former

dubs

>>20
C99 doesn't count.

>>17
What about stdbool.h?

>>23
Good thing then, C11 has booleans too.

>>25

C11

U MENA (C++)--?

>>20,24,25
C99 and all the shit after it isn't C.

>>27
C89 isn't C either. From 1970 to 1988, the function syntax was the same. In 1989, all of a sudden people should be using a completely different syntax for the same thing. I also believe ECMAScript 6 isn't JavaScript.