Preprocessor

[Konstantin8105]

Prototype of preprocessor
Cleaning output Go code
For #232

---

This change is 

[Konstantin8105]

Review status: 10 of 26 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, elliotchance (Elliot Chance) wrote…

I'm not clear what this PR is for exactly?

Cleaning output Go code, see #232

---

ast/ast.go, line 257 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Please move this to ast/utils.go. I want to keep this file as just a controller for the other parse methods.

Done.

---

preprocessor/parse_include_preprocessor_line.go, line 12 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I believe it is allowed for spaces to appear before this, you might want to do this first:

line = strings.TrimSpace(line)

No need. It is always start from symbol "#"

---

program/function_definition.go, line 170 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Why did it need to be changed?

With size_t it is not work.

---

tests/enum.c, line 10 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Why was this removed?

Change for avoid intersection with constants from stdio.h

---

tests/enum.c, line 38 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Also why was this removed?

Change for avoid intersection with constants from stdio.h

---

Comments from Reviewable

[Konstantin8105]

Review status: 10 of 26 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

transpiler/operators.go, line 73 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Remove this and the above debugging code.

Done

---

tests/math.c, line 14 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Fix this and all of the tests below.

Done.

---

Comments from Reviewable

[Konstantin8105]

PR ready for review.

Before review - Please see #232.

[elliotchance]

Reviewed 2 of 5 files at r3, 1 of 8 files at r5, 3 of 9 files at r6, 11 of 14 files at r7, 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

ast/ast_test.go, line 44 at r8 (raw file):

	cond.AddChild(&ImplicitCastExpr{})
	cond.AddChild(&ImplicitCastExpr{})
	s := Atos(cond)

Can you also check the value of s?

---

linux/ctype.go, line 92 at r8 (raw file):

// IsAlpha handles isalpha().
func IsAlpha(_c int) int {

Why do these need to be implemented now? Can they not be understood from parsing the header files?

---

noarch/ctype.go, line 4 at r8 (raw file):

package noarch

// Constants of `ctype.h`

I understand that these come from ctype.h - but why are they being implemented now?

---

program/function_definition.go, line 170 at r5 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

With size_t it is not work.

I see you reverted the change. Does that mean that size_t is now working?

---

program/function_definition.go, line 78 at r8 (raw file):

	/*
	   See https://opensource.apple.com/source/xnu/xnu-344.49/osfmk/libsa/ctype.h.auto.html
	*/

Always use single line comments //. It makes the comments more clear and more diff friendly.

---

program/function_definition.go, line 93 at r8 (raw file):

	"int tolower(int) -> linux.ToLower",
	"int isascii(int) -> linux.IsAscii",
	"int toascii(int) -> linux.ToAscii",

There were tests before that used these functions. Why were they working before and now they need to be implemented in Go?

Generally we want to avoid implementing any C stdlib functions. Only functions that are so low level they cannot be understood by the headers. Otherwise we are not being true to a transpiler and we will have to implement many functions that work differently on different platforms.

---

tests/enum.c, line 10 at r5 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

Change for avoid intersection with constants from stdio.h

I see - if they belong to stdio you should be able to remove them and the test still passes?

---

transpiler/variables.go, line 52 at r8 (raw file):

		"_ISspace", "_ISprint", "_ISgraph", "_ISblank", "_IScntrl",
		"_ISpunct", "_ISalnum"}
	for _, c := range ctypeConstants {

Once again, why has the implementation of these changed?

---

types/cast.go, line 72 at r8 (raw file):

	// implementation type `size_t` from `stdio.h`:
	if fromType == "size_t" {

Why can't it register these types from the header? There would be many more types that could exist.

---

Comments from Reviewable

[Konstantin8105]

Review status: 27 of 30 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

ast/ast_test.go, line 44 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Can you also check the value of s?

Now, format of output in value s is not rigid. So, not clear - How to check?

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Why do these need to be implemented now? Can they not be understood from parsing the header files?

No. Preprocessor anf transpiler are sync to avoid parsing the header files.
So, We have to all types, function, constants from header file add in noarch(for example).
It is help to create clear Go code without system types, ...
Also, it is help to easy implement new feature of language. For example: Now, I cannot finilaze PR "struct typedef without name #270 ", language feature is work, but structures typedef without name in system header is too many and not clear - what I have to do with all that types.

That way help to easy create external API of header without transpilation of deep structure.
External API of C is standart - http://en.cppreference.com/w/c/header , but deep structure can be anything.

---

noarch/ctype.go, line 4 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I understand that these come from ctype.h - but why are they being implemented now?

see upper.

---

program/function_definition.go, line 170 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I see you reverted the change. Does that mean that size_t is now working?

Yes, I found more better solution - add in types list : types/resolve.go

---

program/function_definition.go, line 78 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Always use single line comments //. It makes the comments more clear and more diff friendly.

Done.

---

program/function_definition.go, line 93 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

There were tests before that used these functions. Why were they working before and now they need to be implemented in Go?

Generally we want to avoid implementing any C stdlib functions. Only functions that are so low level they cannot be understood by the headers. Otherwise we are not being true to a transpiler and we will have to implement many functions that work differently on different platforms.

see upper.

---

tests/enum.c, line 10 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I see - if they belong to stdio you should be able to remove them and the test still passes?

No. User code is cannot have variables with same name like in system header.
Also, please see my first comment in that review.

---

transpiler/variables.go, line 52 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Once again, why has the implementation of these changed?

Please see my first comment in that review.
All contants are not include in output Go code, they must included in package noarch .

---

types/cast.go, line 72 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Why can't it register these types from the header? There would be many more types that could exist.

Now, found better have - move to types/resolve list

---

Comments from Reviewable

[Konstantin8105]

Review status: 27 of 30 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

No. Preprocessor anf transpiler are sync to avoid parsing the header files.
So, We have to all types, function, constants from header file add in noarch(for example).
It is help to create clear Go code without system types, ...
Also, it is help to easy implement new feature of language. For example: Now, I cannot finilaze PR "struct typedef without name #270 ", language feature is work, but structures typedef without name in system header is too many and not clear - what I have to do with all that types.

That way help to easy create external API of header without transpilation of deep structure.
External API of C is standart - http://en.cppreference.com/w/c/header , but deep structure can be anything.

One more point - if we use argument c2go transpile -keep-unused ... then all will work like in past with add system structures, constants and ... in output Go code.

---

Comments from Reviewable

[Konstantin8105]

Review status: 27 of 30 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

One more point - if we use argument c2go transpile -keep-unused ... then all will work like in past with add system structures, constants and ... in output Go code.

For example - we can see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf ISO/IEC 9899 on page detail of implementation <stdlib.h>, but deep inside structure platform depended.

---

Comments from Reviewable

[Konstantin8105]

Review status: 27 of 30 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

linux/ctype.go, line 92 at r8 (raw file):

No. Preprocessor anf transpiler are sync to avoid parsing the header files.
So, We have to all types, function, constants from header file add in noarch(for example).
It is help to create clear Go code without system types, ...
Also, it is help to easy implement new feature of language. For example: Now, I cannot finilaze PR "struct typedef without name #270 ", language feature is work, but structures typedef without name in system header is too many and not clear - what I have to do with all that types.
That way help to easy create external API of header without transpilation of deep structure.
External API of C is standart - http://en.cppreference.com/w/c/header , but deep structure can be anything.

One more point - if we use argument c2go transpile -keep-unused ... then all will work like in past with add system structures, constants and ... in output Go code.
For example - we can see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf ISO/IEC 9899 on page detail of implementation <stdlib.h>, but deep inside structure platform depended.

---

Comments from Reviewable

[elliotchance]

Reviewed 7 of 7 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ast/ast_test.go, line 44 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

Now, format of output in value s is not rigid. So, not clear - How to check?

You could match part of the output with a substring, or perhaps use a regexp to match part of the start/middle/end? Basically you want to prove that if Atos totally breaks and returns something very different that this test will fail.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

No. Preprocessor anf transpiler are sync to avoid parsing the header files.
So, We have to all types, function, constants from header file add in noarch(for example).
It is help to create clear Go code without system types, ...
Also, it is help to easy implement new feature of language. For example: Now, I cannot finilaze PR "struct typedef without name #270 ", language feature is work, but structures typedef without name in system header is too many and not clear - what I have to do with all that types.
That way help to easy create external API of header without transpilation of deep structure.
External API of C is standart - http://en.cppreference.com/w/c/header , but deep structure can be anything.

One more point - if we use argument c2go transpile -keep-unused ... then all will work like in past with add system structures, constants and ... in output Go code.
For example - we can see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf ISO/IEC 9899 on page detail of implementation <stdlib.h>, but deep inside structure platform depended.

This is not an acceptable solution. You are correct that header files can be very different, that's exactly why they need to be understood by the transpiler. The output programs have to work as the original compiler intended, and not based of a separate spec.

The main point is that c2go should work exactly the same with -keep-unused on or off with the only difference being that some entities are omitted from the output file.

Maybe take a step back and think about how you can remove unused entities only after the full Go AST has been generated (without changing anything before that point).

---

Comments from Reviewable

[Konstantin8105]

Review status: 24 of 30 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

ast/ast_test.go, line 44 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

You could match part of the output with a substring, or perhaps use a regexp to match part of the start/middle/end? Basically you want to prove that if Atos totally breaks and returns something very different that this test will fail.

Add one more checking.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This is not an acceptable solution. You are correct that header files can be very different, that's exactly why they need to be understood by the transpiler. The output programs have to work as the original compiler intended, and not based of a separate spec.

The main point is that c2go should work exactly the same with -keep-unused on or off with the only difference being that some entities are omitted from the output file.

Maybe take a step back and think about how you can remove unused entities only after the full Go AST has been generated (without changing anything before that point).

In my point of view , all entities from system headers must be in package noarch, but not in user source. Please clarify your point of view.
May be we can change from present to another:
case 1 - user run c2go without flag -keep-unused: Transpiler run 2 parallel process - first(User flow): transpile only user source; second(System flow): transpile only system header source without adding to output Go and created just for compare entities from system header with noarch object. If "System flow" don't found any entities from noarch, then transpiler run again like with flag -keep-unused.
case 2 - user run c2go with flag -keep-unused: present design of transpiler.

---

Comments from Reviewable

[Konstantin8105]

Review status: 23 of 30 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

In my point of view , all entities from system headers must be in package noarch, but not in user source. Please clarify your point of view.
May be we can change from present to another:
case 1 - user run c2go without flag -keep-unused: Transpiler run 2 parallel process - first(User flow): transpile only user source; second(System flow): transpile only system header source without adding to output Go and created just for compare entities from system header with noarch object. If "System flow" don't found any entities from noarch, then transpiler run again like with flag -keep-unused.
case 2 - user run c2go with flag -keep-unused: present design of transpiler.

Or : I can change my design of Preprocessor for solving problem with dublicate of system header and remove all about -keep-unused.

---

Comments from Reviewable

[elliotchance]

Reviewed 1 of 7 files at r10.
Review status: 24 of 30 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

Or : I can change my design of Preprocessor for solving problem with dublicate of system header and remove all about -keep-unused.

The misunderstanding might be what system headers actually are. There is nothing special about system headers, they work exactly the same as any other source file that exists.

c2go is a transpiler, not an abstraction layer or interpreter. Just like the clang compiler itself it does not differentiate where functions are defined.

There are only two cases when functions must be implemented in noarch/darwin/linux:

1. Any function that does not have an implementation (the header file only provides a prototype). They are not many of these and they are extremely low level functions that must be written in assembly (linked after compilation) and cannot be defined in the C files.

2. A function that contains code that cannot be understood by c2go right now. A good example of this is the enum values in ctype that contain compile time expressions that are not evaluated yet in c2go. The function could also have special compiler extensions or have some other reason that we need to implement a Go version. However, these are temporary, when c2go understands how to transpile these functions the definitions can be removed from Go.

Removing the unused entities is definitely a good idea. But it must be done as a final stage (right before it renders the Go) and not interfere with any other process.

It is a significant change and will be released as a new version, but it must be done in the right way.

---

Comments from Reviewable

[Konstantin8105]

Review status: 24 of 30 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

linux/ctype.go, line 92 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

The misunderstanding might be what system headers actually are. There is nothing special about system headers, they work exactly the same as any other source file that exists.

c2go is a transpiler, not an abstraction layer or interpreter. Just like the clang compiler itself it does not differentiate where functions are defined.

There are only two cases when functions must be implemented in noarch/darwin/linux:

1. Any function that does not have an implementation (the header file only provides a prototype). They are not many of these and they are extremely low level functions that must be written in assembly (linked after compilation) and cannot be defined in the C files.

2. A function that contains code that cannot be understood by c2go right now. A good example of this is the enum values in ctype that contain compile time expressions that are not evaluated yet in c2go. The function could also have special compiler extensions or have some other reason that we need to implement a Go version. However, these are temporary, when c2go understands how to transpile these functions the definitions can be removed from Go.

Removing the unused entities is definitely a good idea. But it must be done as a final stage (right before it renders the Go) and not interfere with any other process.

It is a significant change and will be released as a new version, but it must be done in the right way.

May be another solution:
user run c2go transpile file.c and the end we generate 2 files:

1. file.go - transpiling of user code.
2. system.go(Name like example) - all transpile entities from system headers.

---

Comments from Reviewable