remy / vscode-nextbasic

VS Code tools for NextBASIC
https://marketplace.visualstudio.com/items?itemName=remysharp.nextbasic
8 stars 2 forks source link

Export to .bas (when "strip comments enabled") renumbers exported file #51

Closed NealeTools closed 3 months ago

NealeTools commented 4 months ago

v1.11.3 RunInCSpect option works fine, but Export to binary .bas macro (using the same VSCode .txt file) renumbers the output, including any references to GOTO/SUB jump destinations (which then breaks them too):

autostart 1

5 RUN AT 3: PRINT "<<< MULTI_BANK_OUTPUT >>>" 10 LOAD "BANK20" BANK 20 15 LOAD "BANK21" BANK 21 17 LOAD "BANK22" BANK 22 20 PRINT "BASIC ON THE..." 30 BANK 20 GOSUB 30 35 BANK 21 GOSUB 30 37 BANK 22 GOSUB 30 40 GOTO 20

bankfile BANK20

30 PRINT "20 ZX" 40 RETURN

bankfile BANK21

30 PRINT "21 SPECTRUM" 40 RETURN

bankfile BANK22

30 PRINT "22 NEXT!" 40 RETURN

This is the result in CSpect after loading the exported .bas file It autoruns, but exits at line 50 (which didn't exist the listing as written, but has been renumbered to 10, 20 etc) because it is trying to jump to BANK 20 line 50, rather than BANK 20 line 30. (Note the exported BANK files themselves do not appear to have been renumbered.)

image

Exported .bas that has been renumbered:

image

BANK 20 as loaded in CSPect and bas2txt:

image
remy commented 4 months ago

Okay, so there's something you're seeing that I'm not - here's a screenshot, the renumbering isn't happening for me:

SCR-20240310-jcxe

Untitled.bas is the result of the export command in vscode. Then I tested with both the command line bas2txt and the vscode import command, both result in the correct line numbers.

The sequence I used was -

remy commented 4 months ago

I should add, when I run with cspect, the result is the same - I don't have any renumbering happening…

It shouldn't matter, but worth removing all .bas files just to clear out any (potential) confusion with other files? Then repeat?

NealeTools commented 4 months ago

I did that (cleaned up directory). And in my workflow I get the same result I was seeing after opening Untitled.bas and converting:

image
NealeTools commented 4 months ago

Are there possibly two export to binary commands? I'm using SHIFT+OPTION+E

remy commented 4 months ago

Nope, shift+opt+e is just bound to the nextbasic.export command.

I'm trying to update the tool on zx.remysharp.com to see if we can get out of vscode to find out if that's the issue (maybe some config that I'm missing…) - but I've got some build problems so I'll let you know where I get to.

If you have node installed (and you want to do more testing - https://nodejs.org/en) - you could install node, then on the command line run: npm install --global txt2bas, then with your source file (let's assume it's called test.bas.txt) run:

txt2bas -i text.bas.txt | bas2txt

This will convert it and then import and spit out the plain text. If you still have renumbering, then upload the contents of text.bas.txt, if you don't have renumbering, then it's vscode somewhere…

NealeTools commented 4 months ago

This works fine: gdsc021754:GeneralCode mn98$ txt2bas -i FirstTest.bas | bas2txt

autostart 1

5 RUN AT 3: PRINT "<<< MULTI_BANK_OUTPUT >>>" 10 LOAD "BANK20" BANK 20 15 LOAD "BANK21" BANK 21 17 LOAD "BANK22" BANK 22 20 PRINT "BASIC ON THE..." 30 BANK 20 GO SUB 30 35 BANK 21 GO SUB 30 37 BANK 22 GO SUB 30 40 GO TO 20

But this is also true of the RunInCSpect version (no aberrant renumbering). The only one that renumbers is the Export to binary .bas.

remy commented 4 months ago

I'm still a bit stumped as to how this is happening. Are you able to reduce it right down to replicate?

NealeTools commented 4 months ago

I'll have a look again tonight. I may also just try a VSCode etc reinstall...

NealeTools commented 4 months ago

OK - just tried on my work machine. It is fine! So it has to be a problem with my laptop install. I'll rework that tonight an see if it fixes it!

image
remy commented 4 months ago

Weird.

I've pushed a release now (which will sort out those red lines on the line numbers too).

NealeTools commented 4 months ago

Uninstalled and reinstalled v1.11.4 and still the same result on my laptop...

image
NealeTools commented 4 months ago

@remy OMG I solved it. And I seem to recall this was also a bug from ages and ages ago...

image

Strip Comments during export!!!

image

I had this ticked—which seems to switch on auto-renumber! Hopefully this will be an easy fix!

remy commented 4 months ago

Right, re-opening with that detail 👍

remy commented 4 months ago

I know why it renumbers - because if you have something like "20 REM this is a comment" it renumbers to get that out of the way. I suspect the safest approach is just to remove them and leave gaps in the numbering (which, probably doesn't matter)

NealeTools commented 4 months ago

1) By the way, ticking the "strip comments" option doesn't actually seem to remove comments that use semi colon when doing RunInCSpect ; Is that a bug? Or is this only relevant for the Export to bas version but not the RunInCspect version? 2) Are # lines always stripped, regardless of option ticking? 3) Finally, .asm uses ;comments to denote lines of inline assembly that follow the .asm token. You may want to consider handling that?

remy commented 4 months ago
  1. It's for export - I'll clarify that in the options (unless you think it should apply across both export and test?)
  2. Lines with # aren't comments, they're definitions for the txt2bas to process, so these are always removed in the basic file, but an attempt to restore them happens in the bas2txt.
  3. The .asm is kinda arbitrary, it could be .convert_to_asm, but these dotfiles rely on parsing the source nextbasic tokens to find comments in the code, slurp them up and convert to bytecode. So this is down to the author to ensure these remain.

What I might do is to add a detail to the notification that comes up to state the comments have been stripped in the export

NealeTools commented 4 months ago
  1. It perhaps would be more consistent if both export and test behaved the same. After all, the RunInCSpect test version is also exporting to the .bas format.
  2. Understood (now). I think I've had this ticked by default because I thought it was necessary to remove the # lines.
  3. Understood. I like the idea of a notification.

I'm still not clear why stripping lines with REM/; comments in them necessitates a global (or any) renumber. I think stripping comments and renumbering can/should be independently controlled.

remy commented 4 months ago

On the renumbering: that was just a decision I made, and it's wrong. So I'm reversing it and will public the fix (which is why I reopened this issue)

remy commented 3 months ago

Closing as this landed a little while ago in the extension 👍