TakefiveInteractive / TedkOS

Operating System - ECE 391 Takefive Interactive Team
GNU General Public License v2.0
96 stars 17 forks source link

No magic constants #32

Closed markzyu closed 8 years ago

markzyu commented 8 years ago

This is worth 1 point in Checkpoint 3!

markzyu commented 8 years ago

Actually I was to notify everyone about this... But assignee can't be multiple people... @Tedko @yuwang17

tengyifei commented 8 years ago

Where are they?

markzyu commented 8 years ago

I don't know... Thus we need to search the whole project for them... That's why I labeled it as "help wanted"...

Tedko commented 8 years ago

ok· so if we declare they in macro then it's ok? On Sat, Nov 14, 2015 at 16:46 C4Phone notifications@github.com wrote:

I don't know... Thus we need to search the whole project for them... That's why I labeled it as "help wanted"...

— Reply to this email directly or view it on GitHub https://github.com/yuwang17/ece391/issues/32#issuecomment-156752562.

markzyu commented 8 years ago

Yes. It’s also fine if we add a comment about the magic numbers.

On Nov 14, 2015, at 6:01 PM, Han notifications@github.com wrote:

ok· so if we declare they in macro then it's ok? On Sat, Nov 14, 2015 at 16:46 C4Phone notifications@github.com wrote:

I don't know... Thus we need to search the whole project for them... That's why I labeled it as "help wanted"...

— Reply to this email directly or view it on GitHub https://github.com/yuwang17/ece391/issues/32#issuecomment-156752562.

— Reply to this email directly or view it on GitHub https://github.com/yuwang17/ece391/issues/32#issuecomment-156761711.

Tedko commented 8 years ago

I go through all c/cpp/h

Here are magic nums:

klibs/palloc.cpp
klibs/palloc_vars.cpp
drivers/terminal.c //see line 137, line 432
syscall/exec.cpp //line 110
markzyu commented 8 years ago

Thank you.

Tedko commented 8 years ago

@tengyifei

markzyu commented 8 years ago

I am working on it

markzyu commented 8 years ago

@Tedko If you are sure this is all, then please close the issue.