OpenComputers icon indicating copy to clipboard operation
OpenComputers copied to clipboard

Cleanup and fixes in OpenOS.

Open Possseidon opened this issue 5 years ago • 3 comments
trafficstars

Various cleanup and minor fixes all over OpenOS, which showed up with the vscode Lua plugin (a great plugin btw).

The changes include:

  • Removed all trailing whitespace.
  • Made accidental globals local.
  • Removed unused locals (including unused requires).
  • Removed duplicate enter and tab from keyboard.keys table.
  • Removed handling of ... varargs from pairs, as it only takes a single parameter.
  • A fix in rm.lua, where remove and remove_all both call each other, but remove is defined after remove_all.
  • ... and probably a few other things I missed.

Possseidon avatar Oct 31 '20 15:10 Possseidon

I reviewed all the changes. nice cleanup i'm okay with removing the ... on pairs, though i have my reasons. also, nice fix for rm

I need you to rebase these changes for 1.7.10 and not 1.12, which technically means you'll need to cherry pick them over. if you don't want to, no worries, I'm happy to.

payonel avatar May 15 '21 06:05 payonel

Oh damn! I thought this was just gonna rot away. Glad you finally took a look at it! ^^ Yeah, go ahead and cherry-pick them yourself. There probably won't be any conflicts, but I haven't done a whole lot of cherry-picking yet.

And while we're at it. I noticed a slight difference from how coroutine.wrap() gets wrapped in /boot/01_process.lua. The official Lua implementation propagates errors, i.e. if the internally called coroutine.resume() returns false. The current implementation in OpenOS just ignores wether it was an error or an actual yield/return with select(2, ...). Was there a reason for this change? The docs explicitly state, that the whole coroutine wrapping should be transparent and any noticeable difference could be considered a bug.

Thus, if you're ok with it, I'll create another PR to fix this. (This time on the 1.7.10 branch, sorry for the inconvenience there...)

Edit: Actually looking at the blame for that part, yes there seem to have been some changes in the past to get some other stuff working and it used to be "correct". Hmmm...

Possseidon avatar May 15 '21 08:05 Possseidon

@Possseidon I have thousands of unit tests for OpenOS - so I have a fair bit of confidence in making low level changes. I'll take a look at wrap so determine how best to adjust it. Thanks for reporting it. And I'll cherry pick these changes in. That means I'll be closing this PR but technically using your work. I'll make sure you're credited in the release notes for it. I appreciate it

payonel avatar May 15 '21 13:05 payonel

Merged (minus the ... on pairs removal).

asiekierka avatar Jun 03 '23 13:06 asiekierka