Refactored ast nodes into separate package #45

zoeyfyi · 2017-04-16T17:30:25Z

All the ast nodes seemed like a good candidate for separating out into a separate package so the root folder is cleaner.

I also merged Render() and RenderLine() into one Render() function since having both seemed redundant, this should also cover #38. This means we can get rid of all the slow reflection since all nodes now implement the ast.Node interface.

TODO

Remove reflection code from buildTree
Fix issue with the following variable declarations not working after the first test: _LIB_VERSION, _IO_2_1_stdin_, _IO_2_1_stdout_, _IO_2_1_stderr_, stdin, stdout, stderr
Fix incorrect coverage

If this is not how you want the project structured feel free to discard this PR.

This change is

Update

codecov · 2017-04-16T17:53:39Z

Codecov Report

Merging #45 into master will decrease coverage by 19.72%.
The diff coverage is 73.2%.

@@             Coverage Diff             @@
##           master      #45       +/-   ##
===========================================
- Coverage   93.32%   73.59%   -19.73%     
===========================================
  Files          62       66        +4     
  Lines        1318     1530      +212     
===========================================
- Hits         1230     1126      -104     
- Misses         63      378      +315     
- Partials       25       26        +1

Impacted Files	Coverage Δ
ast/util.go	`71.42% <ø> (ø)`
ast/function_definition.go	`100% <ø> (ø)`
ast/enum.go	`0% <0%> (ø)`
ast/qual_type.go	`0% <0%> (ø)`
ast/typedef.go	`0% <0%> (ø)`
ast/typedef_type.go	`0% <0%> (ø)`
ast/record_type.go	`0% <0%> (ø)`
ast/function_proto_type.go	`0% <0%> (ø)`
ast/record.go	`0% <0%> (ø)`
ast/restrict_attr.go	`0% <0%> (ø)`
... and 78 more

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 8952a01...802065d. Read the comment docs.

elliotchance · 2017-04-17T02:04:53Z

Hi @bongo227 - interestingly I did actually have all of these files in an ast package in earlier versions. I moved them out to the root package to get the code coverage working with the integration tests. I would much rather have several small packages that one massive main package.

For each TODO:

Nothing needs to be said on this, sounds good!
I ran in a similar issue on ctype.h #30 . Global state is evil and now that the integration tests are working (thanks for that!) we should really refactor out the global state, or at least move all the initialisation to a single method that we can repeatedly call cleanly for now.
Either my laziness or haste I did not check if code coverage works across packages, but it looks like it can with the -coverpkg.

elliotchance · 2017-04-17T04:49:14Z

Reviewed 66 of 125 files at r1.
Review status: 36 of 66 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

main.go, line 15 at r1 (raw file):

	"go/format"

	"github.com/bongo227/c2go/ast"

Can we use a relative package name here?

ast/function_decl.go, line 57 at r1 (raw file):

	out := bytes.NewBuffer([]byte{})

	ast.functionName = strings.TrimSpace(n.Name)

This trim is not necessary anymore. It was ported from the original Python.

Comments from Reviewable

elliotchance · 2017-04-17T05:44:45Z

Reviewed 59 of 125 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

ast/common.go, line 51 at r1 (raw file):

}

// func Render(out *bytes.Buffer, node interface{}, functionName string, indent int, returnType string) {

You can remove this now.

ast/compound_stmt.go, line 35 at r1 (raw file):

}

func (n *CompoundStmt) RenderLine(out *bytes.Buffer, functionName string, indent int, returnType string) {

Is this supposed to be here?

ast/resolve.go, line 77 at r1 (raw file):

}

func resolveType(ast *Ast, s string) string {

Not for this PR. But I would like to make the whole type resolving system a separate package (resolve?) with its own unit tests. This should also make it easier to handle more complex and state-oriented type resolutions like function pointers.

Comments from Reviewable

zoeyfyi · 2017-04-17T10:22:35Z

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

main.go, line 15 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Can we use a relative package name here?

Relative import paths are highly discouraged, I was going to switch these out at the end with the correct path.

Comments from Reviewable

elliotchance · 2017-04-17T11:11:42Z

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

main.go, line 15 at r1 (raw file):

Previously, bongo227 (Ben Sheffield) wrote…

Relative import paths are highly discouraged, I was going to switch these out at the end with the correct path.

That's fair enough. Or maybe it would be easier to rename your directory path to be the same when this pops up in the future.

Comments from Reviewable

elliotchance · 2017-04-17T13:52:50Z

Reviewed 55 of 125 files at r1, 59 of 59 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

ast/ast.go, line 52 at r2 (raw file):

type Node interface {
	render(ast *Ast) (string, string)
	AddChild(node Node)

It occurs to me that not all node types will have children, like a StringLiteral. For now it's easier to assume they all do, but maybe something like this?

type NodeWithChildren interface {
    *Node
    AddChild(node Node)
}

However, this might be a bit too pedantic and maybe more appropriate for a separate PR where each node can be checked if it can indeed have children.

ast/common.go, line 51 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

You can remove this now.

I'm not sure if you have used reviewable before or if you simply haven't responded to these yet. Discussions stay open until you respond (notice the handy "Done" button).

Comments from Reviewable

zoeyfyi · 2017-04-17T14:07:14Z

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

main.go, line 15 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

That's fair enough. Or maybe it would be easier to rename your directory path to be the same when this pops up in the future.

Done.

ast/ast.go, line 52 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

It occurs to me that not all node types will have children, like a StringLiteral. For now it's easier to assume they all do, but maybe something like this?
type NodeWithChildren interface {
    *Node
    AddChild(node Node)
}
However, this might be a bit too pedantic and maybe more appropriate for a separate PR where each node can be checked if it can indeed have children.

Is their a spec for the clang AST anywhere? I haven't managed to find one. I agree that a separate PR is probably best for this one.

ast/common.go, line 51 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I'm not sure if you have used reviewable before or if you simply haven't responded to these yet. Discussions stay open until you respond (notice the handy "Done" button).

Done.

ast/compound_stmt.go, line 35 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Is this supposed to be here?

Done.

ast/function_decl.go, line 57 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This trim is not necessary anymore. It was ported from the original Python.

Done.

ast/resolve.go, line 77 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Not for this PR. But I would like to make the whole type resolving system a separate package (resolve?) with its own unit tests. This should also make it easier to handle more complex and state-oriented type resolutions like function pointers.

Done. I created an issue.

Comments from Reviewable

elliotchance · 2017-04-17T15:46:02Z

@bongo227 this is a really great change. I'm happy to merge this in if your ready?

Codecov is failing because of the drop in coverage, that's understandable since all the new code. That will be cleaned up over time.

zoeyfyi · 2017-04-17T15:51:54Z

Their is still a problem with those variable declarations (I just ignore them to parse the tests). This can probably be fixed when resolve is refactored out so I would say merging now would be fine.

elliotchance · 2017-04-17T16:00:20Z

Cheers: https://github.com/elliotchance/c2go/releases/tag/v0.8.7

zoeyfyi added 3 commits April 16, 2017 13:10

Merge pull request #2 from elliotchance/master

c7b9916

Update

Refactored ast nodes into seperate package.

75540be

Fixed issue with ast test.

f4e30d3

elliotchance added a: medium d: refactor labels Apr 17, 2017

zoeyfyi added 5 commits April 17, 2017 12:41

Added -coverpkg to correct code coverage.

19122f3

Removed unused code.

cdbb56e

Fixed import path.

6c46fb8

Removed reflection from buildTree.

dab24cb

Fixed test coverage.

802065d

elliotchance mentioned this pull request Apr 17, 2017

Only export interfaces from ast package #51

Open

elliotchance merged commit 9bb200e into elliotchance:master Apr 17, 2017

elliotchance modified the milestone: v0.8.x Apr 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored ast nodes into separate package #45

Refactored ast nodes into separate package #45

zoeyfyi commented Apr 16, 2017 •

edited by elliotchance

Loading

codecov bot commented Apr 16, 2017 •

edited

Loading

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

Refactored ast nodes into separate package #45

Refactored ast nodes into separate package #45

Conversation

zoeyfyi commented Apr 16, 2017 • edited by elliotchance Loading

codecov bot commented Apr 16, 2017 • edited Loading

Codecov Report

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 17, 2017

elliotchance commented Apr 17, 2017

zoeyfyi commented Apr 16, 2017 •

edited by elliotchance

Loading

codecov bot commented Apr 16, 2017 •

edited

Loading