pawn-lang / compiler

Pawn compiler for SA-MP with bug fixes and new features - runs on Windows, Linux, macOS
Other
306 stars 72 forks source link

Fixes for function and variable declarations #631

Open Daniel-Cortez opened 3 years ago

Daniel-Cortez commented 3 years ago

What this PR does / why we need it:

This PR does the following:

static Func2(); // Class specifier "static" is introduced. forward stock Func2(); // Class specifier "stock" is introduced; now the definition is required to have // both keywords "static" and "stock". stock static Func2(){} // OK (both class specifiers "static" and "stock" are in place).

  * * After the function is defined, subsequent "forward" re-declarations can't introduce any new specifiers.
```Pawn
static stock Func(){}
forward public Func(); // Error (function wasn't defined as "public").

Which issue(s) this PR fixes:

Fixes #621, #622, #623, #624, #625, #635

What kind of pull this is:

Additional Documentation:

Y-Less commented 3 years ago

Those missing specifier rules are causing YSI to stop building. I know I usually say that breaking YSI isn't a blocker for new compiler versions because it does a lot of silly things, but I'm not entirely sure this is a silly thing. If a forward declaration has stock, it seems more sensible to me that the stock rule is added to the function, regardless of what the definition then says. Leads to some interesting things like:

MAKE_STOCK(func);

func()
{
}
Daniel-Cortez commented 3 years ago
MAKE_STOCK(func);

func()
{
}

Hmm... haven't seen such non-trivial application of keyword stock before (am I understanding this correctly that MAKE_STOCK is a macro that resolves into something like stock <name>()?), but I guess it can be useful. I'll see if I can relax those new rules for that specifier.

EDIT: OK, done. Also rebased the changes to the current dev so it would be easier to merge them.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity.

Daniel-Cortez commented 2 years ago

To clarify why I added this new requirement for class specifiers in function implementations, my main concern was that in 3.10.10 (without this PR) class specifiers either had no effect in function declarations (see #624) or could even make the compiler generate invalid code (#621), so this new requirement was supposed to point users at the parts of their code that could cause problems in compiler versions up to 3.10.10.

@Y-Less I already removed that rule for specifier stock, as it couldn't impact generated code (it only inhibits warning 204 in case the function is unused), but do you want me to remove it for static and public as well?

Y-Less commented 2 years ago

I'm actually not sure what the most sensible result is here. It seems there are equally good arguments for "all specifiers are additive", so this declares a static stock function:

forward static Func();
forward stock Func();
Func()
{
}

"all specifiers must build", so only this is valid:

forward static Func();
forward static stock Func();
static stock Func()
{
}

"only the definition has specifiers":

forward Func();
forward Func();
static stock Func()
{
}

In that I don't think any one of them is obviously more "correct" than the others. Having said that, I'd vote for the first one as it gives the most options. Regardless of which is chosen the rule should be applied evenly to stock, static, and public, not just having one special-cased as you did for stock.

Y-Less commented 2 years ago

There is another option - remove old-style declarations, but that's a bigger change.

Y-Less commented 2 years ago

Or rahter that's a breaking change.