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

Misc. minor fixes #423

Merged
merged 10 commits into from
Jun 17, 2019
Merged

Misc. minor fixes #423

merged 10 commits into from
Jun 17, 2019

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Jun 6, 2019

What this PR does / why we need it:

Fixes for various minor bugs that weren't worth making a separate PR for each of them:

  • Fixed invalid formula for calculating array size (in characters) in function hier1().
    The cell size in bits was hardcoded as 32 (should be sizeof(cell)*8), which could lead to generation of invalid code for array boundary checks if the Pawn toolkit was configured with 16- or 64-bit cell size.
    Of course, this bug doesn't affect SA-MP (which is 32-bit), but I think it's still good to have this fix here.

  • Fixed invalid type cast in funcstub().
    See refactoring of the codebase #296 (comment).

  • Fixed OOB array access in encode_cell().
    This one seems to originate from a commit that I made a year and a half ago (sorry!)

  • Fixed memory leaks in plungequalifiedfile().
    Special thanks goes to @YashasSamaga for reporting the bug, as well as providing a fix, though I slightly changed that fix in the following ways:

    • Moved free(path) before error(103), because path won't be freed otherwise. This is because on fatal errors (1xx) error() returns (via longjmp()) directly to an error buffer set in pc_compile, so the code after such error() call is never executed.
    • For the same reason I removed two returns, as they are pretty much useless after error(<fatal error code>).
  • Removed NULL pointer checks before the uses of function free().
    free() does nothing if the pointer is NULL, so any NULL checks before the use of free() are redundant.

  • Fixed invalid uses of realloc() in several places.
    The old pointer could have got leaked if the function failed to resize the old memory block or allocate a new one.

  • Simplified the code in function pushstk() by replacing a group of calls to malloc()/memcpy()/free() with one realloc().

  • Removed function do_switch() (sc6.c).
    Apparently this function was a duplicate of do_jump(); it didn't implement any extra checks, so its only difference from do_jump() was in its name.

  • Simplified string emptiness checks.
    Using strlen(<string>)==0 to identify empty strings is inefficient because if the string is not empty strlen() traverses through the whole string to get its length.

  • Fixed potential NULL pointer dereferences in functions sc_path() and setconfig().

  • Fixed incorrect error message when compact encoding is ineffective (Incorrect fatal error when compact encoding is ineffective #425).

  • Other minor adjustments.

Which issue(s) this PR fixes:

Fixes #425

What kind of pull this is:

  • A Bug Fix
  • A New Feature
  • Some repository meta (documentation, etc)
  • Other

Additional Documentation:

@Daniel-Cortez Daniel-Cortez requested a review from a team as a code owner June 6, 2019 16:10
@Y-Less
Copy link
Member

Y-Less commented Jun 6, 2019

Looks fine to me, but only from a visual readthrough.

  * For the same reason I removed two `return`s, as they are pretty much useless after `error(<fatal error code>)`.

I couldn't find those returns. Did you change this part? I'd suggest not removing returns after fatal errors, since the c compiler doesn't know that error might not return (it could be marked as noreturn, but it does in some circumstances). Thus the return will cut off certain code paths and allow the compiler compiler to do more when compiling the compiler.

@Daniel-Cortez
Copy link
Contributor Author

I couldn't find those returns. Did you change this part?

No, I meant that I removed them from the code @YashasSamaga added in their original fix.

I'd suggest not removing returns after fatal errors, since the c compiler doesn't know that error might not return (it could be marked as noreturn, but it does in some circumstances). Thus the return will cut off certain code paths and allow the compiler compiler to do more when compiling the compiler.

Makes sense, but I removed those returns not only because they would be unreachable, but also to be more in-line with the existing code (in all the other code there are usually no returns after fatal errors).

@Y-Less
Copy link
Member

Y-Less commented Jun 6, 2019

Oh I see, then fair enough.

Copy link
Contributor

@Zeex Zeex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the empty string check defined as a macro please? it would be more readable IMO

@Daniel-Cortez
Copy link
Contributor Author

@Zeex Done.

@Zeex Zeex self-requested a review June 16, 2019 06:50
@Zeex
Copy link
Contributor

Zeex commented Jun 16, 2019

Thanks, looks good

@Y-Less Y-Less merged commit 0cb9fb3 into pawn-lang:dev Jun 17, 2019
@Daniel-Cortez Daniel-Cortez deleted the misc-fixes branch June 17, 2019 01:03
@Daniel-Cortez Daniel-Cortez restored the misc-fixes branch June 17, 2019 07:52
Daniel-Cortez added a commit to Daniel-Cortez/pawn-3.10 that referenced this pull request Jul 20, 2019
* Misc. minor fixes

* Fix OOB array access in encode_cell()

* Fix memory leaks in plungequalifiedfile()

* Remove redundant NULL pointer checks

* Fix invalid uses of realloc()

* Remove 'do_switch()'

* Don't use 'strlen()' to identify empty strings

* Other minor fixes

* Fix incorrect error message when compression is ineffective

* Add macro 'strempty' to identify empty strings
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.

3 participants