bradleyeckert / chad

A self-hosting Forth for J1-style CPUs
Other
24 stars 4 forks source link

Tweaks for clean Linux GCC build #2

Closed uncle-betty closed 3 years ago

uncle-betty commented 3 years ago

Hey Brad,

Thank you for Chad! Its simplicity - including the Verilog CPU - is really refreshing. It's the reason why I've started to learn a bit of Forth. It really seems to be human-scale computing - as you say, a single person is able to understand all parts of the system, the hardware as well as the firmware. A little like the old CP/M machine I own.

I'm building Chad on Ubuntu 18.04 (GCC 7.5.0, Clang 10.0.0) as well as Ubuntu 20.04 (GCC 9.3.0, Clang 9.0.1). I'm using -Wall and -Wextra. This pull request fixes a build failure as well as the warnings I was seeing.

The build error happened in KbHit, which looked a little... C++ish? Here's what I changed for that function:

Further down I changed usleep() to nanosleep(), which supersedes the former in newer POSIX standards. Thus, unistd.h turned into time.h.

So, because of nanosleep() - or, previously, usleep() -, we aren't purely C99, but we also need POSIX. As we thus have a POSIX dependency anyway, I replaced the STDIN constant by fileno(stdin), which also requires POSIX.

Other than that, I added a few "fall through" comments to silence compiler warnings about potentially unwanted fall-throughs. Compilers these days really like to micro-manage the programmer! Well, I guess I asked for it by way of -Wall -Wextra.

Then there was the log color, which is an unsigned value almost everywhere, so I standardized on it.

Finally, there were two instances of ignored return values for fgets() and fread(), respectively. Those are surprisingly hard to silence without doing The Right Thing (i.e., act on the return value). You have to actually do something with the returned value - e.g. compare it to some value - and then (void) it.

All in all, now the code builds fine with -Wall -Wextra -std=c99 -D_POSIX_C_SOURCE=199309 in my setups. The POSIX define is required for nanosleep() and fileno(). Later POSIX standards work as well.

Alternatively, one can just leave out both, the -std=c99 as well as the POSIX define. GCC as well as Clang are pretty generous by default when it comes to what features are enabled.

uncle-betty commented 3 years ago

I just tried my branch on a Mac and the Linux version of KbHit() seems to work there, too. Added __APPLE__ to __linux__.

Also, compiling with -Wall -Wextra now doesn't produce any warnings on OS X, either. (I only have access to an older version, though, 10.13.)

bradleyeckert commented 3 years ago

Hi Uncle, Thank you very much for the tweaks and for reviewing the code. You know a lot more about Linux than I do.

uncle-betty commented 3 years ago

Thanks for the merge, Brad. I'm glad I was able to help. Looking forward to further exploring Chad.