Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libqb refactor - Part 1 #453

Merged
merged 6 commits into from
Feb 16, 2024

Conversation

mkilgore
Copy link
Contributor

As the title says, this is the first of several PRs to split out parts of libqb. For the most part the code is left as-is since it's otherwise hard to verify the changes are correct. You can see the current state of all the changes here, but it's much easier to review the commits in groups.

The main focus of this PR is separating out qbs and mem handling from libqb.cpp. Some other small things are moved and I also turned off C++ warning suppression (except for qbx.cpp. In a later PR I'll turn it on for that as well).

One code change I made was eliminate most direct users of new_error, replacing it with the inline function is_error_pending(). It was going to be too much of a mess to complete along with all the refactoring so I opted not to remove every new_error usage.

A big win here is compiling the qbs functions with -O2. I haven't done many benchmarks of it, but some basic tests I did showed a 2x speed-up in some cases.

Fixes: #146

Moves the qbs, command, and error handling APIs into separate .cpp files
in libqb/src/. This makes only minor changes to the actual code beyond
moving the logic, many global variables are left in place to be dealt
with in further changes.

Fixes: QB64-Phoenix-Edition#146
@RhoSigma-QB64
Copy link
Member

Are you sure going to replace new_error by another function call? Be aware that the C code generated from BAS code also references new_error a lot, eg. in all loop and block constructs.

@a740g
Copy link
Contributor

a740g commented Feb 14, 2024

Are you sure going to replace new_error by another function call? Be aware that the C code generated from BAS code also references new_error a lot, eg. in all loop and block constructs.

I think @mkilgore replaced all instances of new_error with is_error_pending() in qb64pe.bas.

@mkilgore
Copy link
Contributor Author

mkilgore commented Feb 15, 2024

Two clarifications on that:

  1. Replacing new_error usage is a goal, but is not completed in this PR. new_error still exists and is exposed to all the code, so there's nothing broken from that standpoint.
  2. I did replace all the usages of it in qb64pe.bas and libqb.cpp, except for a few which were reading/restoring the value and thus couldn't be replaced with is_error_pending(). I want to make a better API for handing that situation rather than just fiddling with the value, but this refactoring already got large enough so I didn't want to add in anymore new code.

Edit: I guess a third one, it's still an inline function in the header to avoid any kind of performance penalty to calling a function that often, I don't intend to change that.

@RhoSigma-QB64
Copy link
Member

RhoSigma-QB64 commented Feb 15, 2024

There is another issue with that error checking I found yesterday when working on a new function, here simplified:

int16 foo() {
    // do stuff
    if (success) {
        return -1;
    } else {
        error(53); // we can continue from the error MessageBox
        return 0;
    }
}

so after clicking continue in the MB the function correctly returns 0, but now the BASIC part:

IF foo() THEN
    PRINT "Success"
ELSE
    PRINT "Failed"
END IF

this will always print "Success" even when continueing from an error, as the IF foo() THEN is translated (roughly) into if (foo() || new_error) and that logical OR with new_error does compromise the zero return of foo() and makes the condition always true.

This scheme is probably involved in all conditional checks such as WHILE, UNTIL etc. and we really should rethink this.

Oh, and I tried adding a new_error = 0 befor the return, which worked, but also suppressed the MessageBox, which is obviously not triggered by error(), but via the evnt() function which is triggered by that || new_error thing.

@mkilgore
Copy link
Contributor Author

@RhoSigma-QB64 I think it's correct in some respect, it's just not desirable behavior. I believe the continue button acts like a Resume Next, it doesn't follow the code logic but rather just skips to the next "line" in the source. You can get the same silly result when putting in your own Resume Next handler:

image

That said, I don't know why we offer a continue option in the message box that does Resume Next :D The likelihood of it working to any degree is very low IMO.

In QBasic the continue option would simply execute the erroring command again, like a Resume, which is significantly more sensible (but also doesn't accomplish much if it's just going to trigger the same error).

@RhoSigma-QB64
Copy link
Member

RhoSigma-QB64 commented Feb 15, 2024

Ah, RESUME NEXT that makes sense then, I guess I was more on a simple RESUME, but that would only execute the faulty line again and again when continueing, rather than resuming to normal program flow.

@mkilgore
Copy link
Contributor Author

Ah, RESUME NEXT that makes sense then, I guess I was more on a simple RESUME, but that would only execute the faulty line again and again when continueing, rather than resuming to normal program flow.

I mean IMO it wouldn't be unreasonable to do a RESUME instead, that's exactly what QBasic does with the Continue option if you hit an error. It does mean you're likely going to just hit the exact same error again, but the chances are high that Resume Next is not going to do the correct thing anyway and you'll just hit another error elsewhere.

I'm not sure if we want to change that behavior now though, that's probably a separate conversation 🤷

@mkilgore mkilgore merged commit fb145ee into QB64-Phoenix-Edition:main Feb 16, 2024
4 checks passed
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.

Refactor qbs into separate source file(s) in libqb/
3 participants