Skip to content

Conversation

@dkearns
Copy link
Contributor

@dkearns dkearns commented Mar 10, 2021

This a recurring problem throughout the code base that just bit me so I'm testing the waters. Would a PR fixing the boolean handling project-wide be merged?

I have no idea why Mac's get special treatment here or why os$ isn't set to "MAC"? I can't see where adding that would cause a problem.

@flukiluke
Copy link
Contributor

Frankly the whole thing seems janky. Lines like IF os$ = "LNX" THEN extension$ = "" seem to imply os$ = "LNX" holds true on Mac systems?

Doing a brief search through to look at how the values are used, I get the impression os$ = "LNX" is really used to check for 'Unix-like' system, mainly dealing with things like / vs \ in paths. For no good reason, there's also a few calls that ignore os$ entirely and do INSTR(_OS$, ....

Finally there's this block, down around line 12027:

o$ = LCASE$(os$)
win = 0: IF os$ = "WIN" THEN win = 1
lnx = 0: IF os$ = "LNX" THEN lnx = 1
mac = 0: IF MacOSX THEN mac = 1: o$ = "osx"

but their use appears to be limited solely to deciding build dependencies.

I suspect that this is what you get when you keep adding platform as layers of special cases - if I had to put money on it I'd say Linux was done first, then OS X was deemed "close enough" to just use the linux conditional in most places; we now have a lovely mess.

I don't really have strong opinions about how it should be organised, except not how it is currently. I'd be willing to accept a change to the conditional value (assuming it doesn't break anything, of course), but know there's a bigger monster lurking beneath the surface.

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