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

Add support of 2 or more C source files. Fixes #63 #258

Merged
merged 155 commits into from Nov 7, 2017
Merged

Add support of 2 or more C source files. Fixes #63 #258

merged 155 commits into from Nov 7, 2017

Conversation

Konstantin8105
Copy link
Contributor

@Konstantin8105 Konstantin8105 commented Oct 21, 2017

See #257, #63


This change is Reviewable

@elliotchance
Copy link
Owner

Review status: all files reviewed at latest revision, 1 unresolved discussion.


tests/multi/four.c, line 1 at r5 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

I check that option in last commit 0aa4566 and dublicates is acceptable for gcc , but not for clang .
Same way to using #ifdef you can see in many project:
https://github.com/DaveGamble/cJSON/blob/master/cJSON.c
https://github.com/sheredom/json.h/blob/master/json.c
Or on discussions:
https://stackoverflow.com/questions/10278494/duplicate-header-files-throughout-source-files
https://en.wikipedia.org/wiki/Include_guard see Use of #include guards

But if that feature is important for you, then I can add that in new Preprocessor #273 .

I'm familiar with how #ifdef works. However it is used to wrap around your own header files in case they are included in multiple places.

I could #include standard header files (like stdio.h) as many times as I want in every file without issue because the header files themselves already have that built in mechanism.

Also, by using #ifdef USE_STDIO you are actually just always not including stdio.h because USE_STDIO is not defined. The pattern you are trying to achieve requires that each file has a different macro name and that you set the macro at least once.

It looks like you are doing this to prevent an error. If it causes an error then this is not an acceptable solution. To be clear the code must compile with:

  1. More than one file.
  2. More than two files must include the same standard header.
  3. No #ifdef trickery to avoid errors.

Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


tests/multi/four.c, line 1 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I'm familiar with how #ifdef works. However it is used to wrap around your own header files in case they are included in multiple places.

I could #include standard header files (like stdio.h) as many times as I want in every file without issue because the header files themselves already have that built in mechanism.

Also, by using #ifdef USE_STDIO you are actually just always not including stdio.h because USE_STDIO is not defined. The pattern you are trying to achieve requires that each file has a different macro name and that you set the macro at least once.

It looks like you are doing this to prevent an error. If it causes an error then this is not an acceptable solution. To be clear the code must compile with:

  1. More than one file.
  2. More than two files must include the same standard header.
  3. No #ifdef trickery to avoid errors.

I will change proprocessor #273 in according to your rules. Please confirm.


Comments from Reviewable

climate change

ParmVarDecl could not match regexp. Fixes #280 (#284)

Fixed struct array initialization. #264 (#283)

Bump version: v0.16.9 Radium 2017-10-30

Function free(). Fixes #123 (#255)

c2go should only output TestApp() when running in test. Fixes #267 (#274)

Add support for multi-dimensional arrays. Fixes #279 (#291)

Line number should be %d, not %s (#295)

Bump version: v0.16.10 Radium 2017-10-31

add more test for ast.Atos
@Konstantin8105
Copy link
Contributor Author

Not ready for review

@Konstantin8105
Copy link
Contributor Author

Not ready for review

@Konstantin8105
Copy link
Contributor Author

🎆 Ready for review

@elliotchance
Copy link
Owner

Reviewed 8 of 31 files at r6, 3 of 15 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


main.go, line 281 at r8 (raw file):

	if outputFilePath == "" {
		// Choose inputFile for creating name of output file
		input := args.inputFiles[len(args.inputFiles)-1]

Why are you using the last input file as the base for the output file?


main_test.go, line 332 at r8 (raw file):

	// Run Go program
	var buf bytes.Buffer
	cmd := exec.Command("go", "run", args.outputFile)

So to be clear, multiple input files still generate a single Go output file?


Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


main.go, line 281 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Why are you using the last input file as the base for the output file?

Yes, It is true. In my point of view, only last file have function main(), that the reason, but in you another think - please clarify.


main_test.go, line 332 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

So to be clear, multiple input files still generate a single Go output file?

Yes. Many C code in input ---> One Go code output.
For present, It is ok(in my point of view). If you see like "Many C code input ---> Many Go code output", then please clarify and create the issue for discussion in detail. But for that PR, please accepted(I am a affaid, because PR with huge commits).


Comments from Reviewable

@elliotchance
Copy link
Owner

Review status: all files reviewed at latest revision, 1 unresolved discussion.


main.go, line 281 at r8 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

Yes, It is true. In my point of view, only last file have function main(), that the reason, but in you another think - please clarify.

I would prefer the first provided file:

input := args.inputFiles[0]

I feel it makes more sense from a CLI point of view.


Comments from Reviewable

@elliotchance
Copy link
Owner

Review status: all files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):
This is a great PR and will be the v0.17.0 release. Can you also update https://github.com/elliotchance/c2go/blob/master/travis/test.sh#L86-L89 so that we transpile both sqlite3 files with the same command.


Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: 13 of 14 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):

Previously, elliotchance (Elliot Chance) wrote…

This is a great PR and will be the v0.17.0 release. Can you also update https://github.com/elliotchance/c2go/blob/master/travis/test.sh#L86-L89 so that we transpile both sqlite3 files with the same command.

Could to merge this PR? Now, need to implement a compiler flag - this another good task for PR :).
Your idea is great, but I want in next PR solve:


main.go, line 281 at r8 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I would prefer the first provided file:

input := args.inputFiles[0]

I feel it makes more sense from a CLI point of view.

Done


Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: 13 of 14 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):

Previously, Konstantin8105 (Konstantin) wrote…

Could to merge this PR? Now, need to implement a compiler flag - this another good task for PR :).
Your idea is great, but I want in next PR solve:

Please confirm design #331


Comments from Reviewable

@Konstantin8105 Konstantin8105 mentioned this pull request Nov 7, 2017
@elliotchance
Copy link
Owner

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


a discussion (no related file):

Previously, Konstantin8105 (Konstantin) wrote…

Please confirm design #331

Yes, that's fair enough.


Comments from Reviewable

@elliotchance elliotchance merged commit 423af0c into elliotchance:master Nov 7, 2017
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