compiler icon indicating copy to clipboard operation
compiler copied to clipboard

Fixes for function and variable declarations

Open Daniel-Cortez opened this issue 3 years ago • 7 comments

What this PR does / why we need it:

This PR does the following:

  • Allows to use a combination of static stock/stock static in new-style function declarations (see #622).
  • Fixes assertion failure when defining static variables with a name starting with @ (see #623).
  • Makes the compiler detect the use of specifier static on variables whose name starts with @ as an invalid combination of class specifiers (see #625).
  • Addresses problems with function class specifiers having an incorrect effect or no effect at all in function declarations (see #621, #624) by introducing the following rules:
    • Function definition must have all class specifiers that were introduced in all previous declarations of the function.
forward public Func1(); // Class specifier "public" is introduced; it will be required in the definition.
Func1(); // OK (previously introduced class specifiers are only mandatory in function definitions,
         // not declarations).
Func1(){} // Error (missing class specifier "public").

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.
static stock Func(){}
forward public Func(); // Error (function wasn't defined as "public").
  • Fixes a bug with the return value tag and array size being expected before the function class specifiers in new-style function declarations (see #635).
  • Fixes a bug with keyword __pragma being expected twice in function/variable declarations starting with keywords public, static and stock.

Which issue(s) this PR fixes:

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

What kind of pull this is:

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

Additional Documentation:

Daniel-Cortez avatar Jan 07 '21 14:01 Daniel-Cortez

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()
{
}

Y-Less avatar May 02 '21 17:05 Y-Less

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.

Daniel-Cortez avatar May 02 '21 18:05 Daniel-Cortez

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

stale[bot] avatar Jan 09 '22 02:01 stale[bot]

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?

Daniel-Cortez avatar Jan 15 '22 15:01 Daniel-Cortez

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 avatar Mar 08 '22 02:03 Y-Less

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

Y-Less avatar Mar 08 '22 02:03 Y-Less

Or rahter that's a breaking change.

Y-Less avatar Mar 08 '22 02:03 Y-Less