Skip to content

Conversation

Daniel-Cortez
Copy link
Contributor

What this PR does / why we need it:

This PR allows the compiler to warn the user if the shift count is too big or negative (both of which are UB), as suggested in #528.
For example, this could be useful when the user tries to store too many bit flags into one variable:

enum ePlayerFlags
{
/*  0 */    pfEmailConfirmed,
/*  1 */    pfTutorialCompleted,
            /* ... */
/* 31 */    pfImprisoned,
/* 32 */    pfHospitalized
};

// ...

player_info[playerid][pFlags] |= (1 << pfHospitalized); // warning 24X: negative or too big shift count

Which issue(s) this PR fixes:

Fixes #

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 21, 2020 14:36
@Y-Less
Copy link
Member

Y-Less commented Jun 21, 2020

What about right shifts?

Also, a related suggestion would be a warning for this:

enum A (<<= 1)
{
	B = 1,
	c00,
	c01,
	c02,
	c03,
	c04,
	c05,
	c06,
	c07,
	c08,
	c09,
	c10,
	c11,
	c12,
	c13,
	c14,
	c15,
	c16,
	c17,
	c18,
	c19,
	c20,
	c21,
	c22,
	c23,
	c24,
	c25,
	c26,
	c27,
	c28,
	c29,
	c30,
	c31,
	c32,
	c33,
	c34,
	c35,
	c36,
	c37,
	c38,
	c39
}

Where the enum shifts overflow.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Jun 21, 2020

Also, a related suggestion would be a warning for this:
(...)
Where the enum shifts overflow.

I wanted to do this initially, but currently the left shift in enum declarations is processed in the same way as multiplication (e.g. <<= 1 is treated as *= 2 internally, <<= 2 becomes *= 4, etc.), so currently there's no way to catch invalid shift values. I think I'll need to rewrite some of the code that handles increments in function decl_enum() (file sc1.c) to make the compiler process the left shift operation in a distinct way (this might also open a way to solution for #540 - in fact, I found that bug when I was trying to make this new warning work on enumerations).

What about right shifts?

But it does work on right shifts.
EDIT: Ah, right, didn't cover them with tests; I'll fix this.

@Y-Less
Copy link
Member

Y-Less commented Jun 21, 2020

But it does work on right shifts.
EDIT: Ah, right, didn't cover them with tests; I'll fix this.

Nice.

@Daniel-Cortez
Copy link
Contributor Author

Updated this PR; now warning 24X is also printed when the specified left shift count is too big

enum (<<= 32) // warning 24X: negative or too big shift count
{
    // ...
};

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Jun 26, 2020

Also, even though I incorporated the fix from #546, I haven't made warning 24X work for shift overflow in enum items yet, as I'm not sure if this warning is correct for such situations.

enum (<<= 1)
{
    c0 = 1,
    c1,
    c2,
    /* ... */
    c31,
    c32, // overflow
};

In this example, technically, c32 is not 1 << 32, it's c31 << 1, so the message "negative or too big shift count" doesn't sound correct in this case.
Maybe it would make sense to add a separate warning for overflow in enum items (and probably not only for left shift, but also for multiplication and addition)? And if so, what would be the best way to phrase the warning message? I'm not a native English speaker, so I'm not sure; for now I'm going with "shift overflow in enum item declaration (symbol \"%s\")", but there's probably a better or more correct way to phrase it.

…t be shown again for the next enum item.

Also rename the variable into more descriptive "warn_overflow".
@Y-Less Y-Less closed this Sep 19, 2020
@Y-Less Y-Less changed the title warning 24X: negative or too big shift count [MERGED] warning 24X: negative or too big shift count Sep 20, 2020
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