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

Suggested fixes for most of the issues I reported #231

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

Conversation

JBontes
Copy link

@JBontes JBontes commented Oct 3, 2017

Note that #224 also fixes a slight issue where the SyntaxNodeNames got out of sync with the SyntaxNodeTypes (My bad). Also note that #224 allows you to put the operators anywhere in the list, as long as all operators are kept together.

Added all remaining fixes in my local version.

This concludes the list of issues for which I have an easy fix.

JBontes and others added 9 commits October 3, 2017 11:04
Added an explicit anKind attribute containing `type` if there is an explicit type.
Record type params (and other parts of the name explicitly in subnodes. Still record the full name in an attribute.
Store all names in a ntName node with a anName attribute for consistency. A name is not a value, so it should not be stored in a value property.
An except-else has very different semantics from an if-else. It needs a specialized node type.
Both class and object can be declared forward.
@RomanYankovsky
Copy link
Owner

Thank you for creating a pull request. It will save a lot of time. I'll review it as soon as possible.

jbontes added 2 commits October 3, 2017 13:34
… time

TOperators.GetItem uses a for loop to extract the correct OperatorsInfo record.
This is inefficient. If you reorder the TSyntaxNodeType enumeration so that the operators are in the same order as OperatorsInfo then we can use a simple in statement.
@JBontes JBontes changed the title Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181 Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181 + optimizations #224 + #225 Oct 3, 2017
Methods can be declared forward in the implementation section.
Also the semicolon after forward is optional as per Turbo Pascal legacy.
@JBontes JBontes changed the title Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181 + optimizations #224 + #225 Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181, #166 + optimizations #224 + #225 Oct 3, 2017
…om SyntaxNodeTypes

The cool thing about this fix is that no changes are need to the rest of the code and it runs with the same performance as the old code.
@JBontes JBontes changed the title Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181, #166 + optimizations #224 + #225 Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181, #166, #232 + optimizations #224 + #225 Oct 3, 2017
@JBontes
Copy link
Author

JBontes commented Oct 3, 2017

You're welcome. I'm just trying to get AST to parse all code correctly so I can use it as a macro/code rewrite tool. Unfortunately my own version has got quite a few alterations that are out of scope for DelphiAST, so I cloned a fresh version and plugged in the changes there.

@JBontes JBontes mentioned this pull request Oct 3, 2017
@JBontes JBontes changed the title Suggested fixes for issues #229, #228, #227, #226, #223, #222, #221, #220, #181, #166, #232 + optimizations #224 + #225 Suggested fixes for issues #237, #236, #235, #234, #233, #232, #229, #228, #227, #226, #223, #222, #221, #220, #181, #166 + optimizations #224 + #225 Oct 3, 2017
… resident external keyword added.

Plus a few other small fixes.
@JBontes JBontes changed the title Suggested fixes for issues #237, #236, #235, #234, #233, #232, #229, #228, #227, #226, #223, #222, #221, #220, #181, #166 + optimizations #224 + #225 Suggested fixes for issues #237, #236, #235, #234, #233, #232, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Oct 5, 2017
Also included are a few minor optimizations.
@JBontes JBontes changed the title Suggested fixes for issues #237, #236, #235, #234, #233, #232, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Suggested fixes for issues #237, #236, #235, #234, #233, #232, #230, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Oct 6, 2017
@JBontes JBontes changed the title Suggested fixes for issues #237, #236, #235, #234, #233, #232, #230, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Suggested fixes for issues #243, #242, #237, #236, #235, #234, #233, #232, #230, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Oct 12, 2017
jbontes added 6 commits October 13, 2017 16:19
This is esp. evident in the following example:
const
  OperatorsInfo: array [0..2] of TOperatorInfo =
    ((Typ: ntAddr;         AssocType: atRight),
     (Typ: ntDeref;        AssocType: atLeft),
     (Typ: ntGeneric;      AssocType: atRight));

All constants are incorrectly listed as one long list, instead of as separate records.
@JBontes JBontes changed the title Suggested fixes for issues #243, #242, #237, #236, #235, #234, #233, #232, #230, #229, #228, #227, #226, #223, #222, #221, #220, #216, #181, #166 + optimizations #224 + #225 Suggested fixes for most of the issues I reported Oct 15, 2017
jbontes and others added 18 commits October 16, 2017 12:43
It can parse arbitrarily complex expressions and also deal with more complex comparisons and floating point numbers.
array of const is a special construct. `const` here is not an identifier.
Added `@@` as a distinct operator.
…ug in parameter processing introduced in the original 217 fix
@JBontes
Copy link
Author

JBontes commented Nov 2, 2018

I'll review it as soon as possible.

I fixed the conflict, have you had time to look at it yet?
No rush :-)

@alex-ilin
Copy link

alex-ilin commented Jun 1, 2021

@RomanYankovsky, will you review the PR already? The suspense is killing me!

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.

3 participants