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

Fixes for function and variable declarations #631

Open
wants to merge 27 commits into
base: dev
Choose a base branch
from

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Jan 7, 2021

What this PR does / why we need it:

This PR does the following:

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").

Which issue(s) this PR fixes:

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

What kind of pull this is:

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

Additional Documentation:

@Y-Less
Copy link
Member

Y-Less commented May 2, 2021

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

…ions and repeated declarations if previous declaration(s) had those specifiers
…on the next pass "forward" declarations won't be required to have those specifiers at first
…ion declarations/definitions.

Also rename "uSTATIC" into "uDECLSTATIC".
This fixes a bug with `__pragma` being expected twice in certain cases.
This fixes another bug with `__pragma` being expected twice in variable declarations starting with specifiers `public`, `static` and `stock`.
…clarations

Example:
  Func(){}
  forward stock Func();
@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented May 2, 2021

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
Copy link

stale bot commented Jan 9, 2022

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

@stale stale bot added the state: stale label Jan 9, 2022
@stale stale bot removed the state: stale label Jan 15, 2022
@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Jan 15, 2022

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
Copy link
Member

Y-Less commented Mar 8, 2022

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
Copy link
Member

Y-Less commented Mar 8, 2022

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

@Y-Less
Copy link
Member

Y-Less commented Mar 8, 2022

Or rahter that's a breaking change.

…ired in following function re-declarations or definition
…ier is introduced in its own function declaration or definition, e.g.:

```
forward static Func();
forward public Func();
```
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