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

Added support for simple pointer arithmetic. Fixes #434 #441

Merged
merged 5 commits into from Dec 11, 2017
Merged

Added support for simple pointer arithmetic. Fixes #434 #441

merged 5 commits into from Dec 11, 2017

Conversation

Konstantin8105
Copy link
Contributor

@Konstantin8105 Konstantin8105 commented Dec 10, 2017

Close #434
See #67


This change is Reviewable

@Konstantin8105
Copy link
Contributor Author

Not ready for review

@Konstantin8105
Copy link
Contributor Author

Ready for review if Ok in travis

@elliotchance
Copy link
Owner

Reviewed 4 of 6 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


transpiler/call.go, line 74 at r2 (raw file):

	// function stdlib.c
	if functionName == "calloc" && len(n.Children()) == 3 {

Can you be sure that len(n.Children()) == 3 means that there are 3 arguments? Maybe you should extract the arguments to be sure. I remember we had a function for returning a specific type of node, but I cannot remember where that code is now...

Something like this?

children := GetNodesOfType(n.Children(), (*ast.Something)(nil))
if functionName == "calloc" && len(children) == 3 {
    // children is not reliable even when the AST has other child nodes
    // that are not arguments.
}

It will make the code more robust if there are any extra children that are not arguments. Also you rely on indexes like 0 and 1 below but the actual parameters may not be located there, use children[0] instead to know it's correct.


transpiler/unary.go, line 173 at r2 (raw file):

// Example of using:
// *(t + 1) = ...
func transpilePointerArith(n *ast.UnaryOperator, p *program.Program) (

This is really awesome! Does this also work with decrementing pointer arithmetic? Like the common loop: while (p-- != NULL) { }


Comments from Reviewable

@elliotchance
Copy link
Owner

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


transpiler/call.go, line 74 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Can you be sure that len(n.Children()) == 3 means that there are 3 arguments? Maybe you should extract the arguments to be sure. I remember we had a function for returning a specific type of node, but I cannot remember where that code is now...

Something like this?

children := GetNodesOfType(n.Children(), (*ast.Something)(nil))
if functionName == "calloc" && len(children) == 3 {
    // children is not reliable even when the AST has other child nodes
    // that are not arguments.
}

It will make the code more robust if there are any extra children that are not arguments. Also you rely on indexes like 0 and 1 below but the actual parameters may not be located there, use children[0] instead to know it's correct.

This is the method: https://github.com/elliotchance/c2go/blob/master/ast/traverse.go#L12

However, that is recursive. You may want to create a similar method that does not recursive.


Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


transpiler/call.go, line 74 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This is the method: https://github.com/elliotchance/c2go/blob/master/ast/traverse.go#L12

However, that is recursive. You may want to create a similar method that does not recursive.

|   |   `-CallExpr 0x1dce580 <col:18, col:40> 'void *'
|   |     |-ImplicitCastExpr 0x1dce568 <col:18> 'void *(*)(unsigned long, unsigned long)' <FunctionToPointerDecay>
|   |     | `-DeclRefExpr 0x1dce4c8 <col:18> 'void *(unsigned long, unsigned long)' Function 0x1dce350 'calloc' 'void *(unsigned long, unsigned long)'
|   |     |-ImplicitCastExpr 0x1dce5b8 <col:25> 'unsigned long' <IntegralCast>
|   |     | `-IntegerLiteral 0x1dce4f0 <col:25> 'int' 5
|   |     `-UnaryExprOrTypeTraitExpr 0x1dce520 <col:27, col:39> 'unsigned long' sizeof 'float'

3 node:

  1. for function identification
  2. for amount of elements
  3. for type size
    I suppose it is like ForStmt - it has 5 children.
    I will add more test for that point.

transpiler/unary.go, line 173 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This is really awesome! Does this also work with decrementing pointer arithmetic? Like the common loop: while (p-- != NULL) { }

Now, it is not support


Comments from Reviewable

@codecov
Copy link

codecov bot commented Dec 11, 2017

Codecov Report

Merging #441 into master will increase coverage by 0.06%.
The diff coverage is 85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #441      +/-   ##
=========================================
+ Coverage   81.03%   81.1%   +0.06%     
=========================================
  Files         145     145              
  Lines        6434    6509      +75     
=========================================
+ Hits         5214    5279      +65     
- Misses        962     968       +6     
- Partials      258     262       +4
Impacted Files Coverage Δ
transpiler/variables.go 81.92% <0%> (ø) ⬆️
program/program.go 80.28% <100%> (ø) ⬆️
types/resolve.go 76.19% <100%> (+0.38%) ⬆️
transpiler/call.go 69.23% <70.58%> (+0.18%) ⬆️
transpiler/unary.go 78.03% <91.07%> (+4.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92b1249...4a241da. Read the comment docs.

@elliotchance
Copy link
Owner

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


tests/array.c, line 232 at r3 (raw file):

		is_true(arr != NULL);
		(void)(arr);
	}

Awesome!


Comments from Reviewable

@elliotchance elliotchance changed the title simple pointer arithmetic Added support for simple pointer arithmetic. Fixes #434 Dec 11, 2017
@elliotchance elliotchance merged commit 130b3d4 into elliotchance:master Dec 11, 2017
@Konstantin8105 Konstantin8105 deleted the pointerArithmetic branch December 26, 2017 07:15
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