Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Rewrite functions, again #214

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Rewrite functions, again #214

wants to merge 5 commits into from

Conversation

winstliu
Copy link
Contributor

@winstliu winstliu commented Mar 7, 2017

🚨 MAJOR WORK-IN-PROGRESS 🚨

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR starts another rewrite of functions and function calls. It fixes a couple of bugs with the existing function implementation, including many cases where function calls would be tokenized as functions instead. The C implementation is based off of language-java's and language-javascript's. In order for C++ functions to tokenize correctly I added a few injections.

Alternate Designs

None.

Benefits

Fixes cases where function calls are tokenized as functions. Operator overloading will be properly supported. Probably some other things as well.

Possible Drawbacks

Pretty major change, and there's barely any C++ spec coverage. Huge possibility of regressions.

Applicable Issues

I'll check later.

/cc @alpyre

# Conflicts:
#	grammars/c.cson
#	spec/c-spec.coffee
@alpyre
Copy link
Contributor

alpyre commented Mar 8, 2017

This looks pretty good overall! Thanks for the work.

The only thing I don't understand is:
Why move c_function_call (with its new name function-call) to $base? There can not be any function calls at base. Only forward declarations or definitions can be there.

}
{
'include': '#function-call'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There can be no function calls here at $base (only func. declarations or definitions can).
Shouldn't this be moved back in to block_innards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly for testing purposes so that I didn't have to constantly create the necessary scaffolding.

'begin': '''(?x)
(?=
[A-Za-z_]\\w* # Type modifier
\\s+
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not require a return type for functions.
That will break:

  • main() (with no int) convention (before C99)
    (which works at the moment as below)
main() {
  /* code */
  return 0;
}
A_Macro()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Then I think this will become trickier, because I've mostly been relying on the return type to figure out if it's a function or a function call.

Copy link
Contributor

@alpyre alpyre Mar 8, 2017

Choose a reason for hiding this comment

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

Here are some tips for you:

  • function patterns at $base are functions (which means they are either func.like macros or function definitions or forward declarations).
  • function patterns inside a block are function-calls
  • function patterns inside a block with return types are function initializations.

You should go according to these.

@alpyre
Copy link
Contributor

alpyre commented Mar 8, 2017

This implementation also breaks "old-style C function declarations":
(which are still supported by new compilers and work at the moment)
old-style

As I coded PR #206 I had supporting these in mind. But forgot to create specs for them. It's my bad, sorry. 😳

@winstliu
Copy link
Contributor Author

winstliu commented Mar 8, 2017

Do function declarations (int max(int, int);) require return types, or can those also be omitted?

@alpyre
Copy link
Contributor

alpyre commented Mar 8, 2017

Function declarations do require return types.
But the problem here is to both cover function like macros and functions at the same time.

I think you should consider the problems I've stated in Issue 189 once more.

I believe the "verbose" implementation you desired should be done by introducing new patterns, not by altering the current one.

@winstliu
Copy link
Contributor Author

FYI: I'm going to have to put this on the backburner for a while to finish up some of my other open PRs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants