Send clang options with "-clang-flag". Fixes #331 #337

Konstantin8105 · 2017-11-08T17:58:47Z

This change is

codecov · 2017-11-08T18:39:34Z

Codecov Report

Merging #337 into master will decrease coverage by 0.23%.
The diff coverage is 57.37%.

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   81.61%   81.38%   -0.24%     
==========================================
  Files         142      143       +1     
  Lines        6032     6080      +48     
==========================================
+ Hits         4923     4948      +25     
- Misses        888      908      +20     
- Partials      221      224       +3

Impacted Files	Coverage Δ
ast/position.go	`82.11% <0%> (-0.61%)`	⬇️
transpiler/transpiler.go	`88.97% <0%> (-1.03%)`	⬇️
ast/ast.go	`98.01% <100%> (+0.01%)`	⬆️
main.go	`54.77% <50%> (-0.55%)`	⬇️
ast/empty_decl.go	`57.89% <57.89%> (ø)`
preprocessor/preprocessor.go	`67.46% <69.56%> (-0.59%)`	⬇️

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 769de31...1e83289. Read the comment docs.

Konstantin8105 · 2017-11-08T18:41:39Z

Not ready for review, but welcome - if you know alternative for macro _GNU_SOURCE on MacOSX.

elliotchance · 2017-11-09T03:43:57Z

Reviewed 10 of 12 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks broke.

a discussion (no related file):
There is a merge conflict from an earlier PR that added some stats to the PR result.

main.go, line 328 at r2 (raw file):

func init() {
	transpileCommand.Var(&clangFlags, "clang-flag", "input C code and specific flags for clang")

"Pass arguments to clang. You may provide multiple -clang-flag items."

main_test.go, line 338 at r2 (raw file):

	for pos, tc := range tcs {
		t.Run(fmt.Sprintf("Test %d", pos), func(t *testing.T) {
			var args = ProgramArgs{}

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

main_test.go, line 372 at r2 (raw file):

func TestTrigraph(t *testing.T) {
	var args = ProgramArgs{}

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

main_test.go, line 405 at r2 (raw file):

func TestExternalInclude(t *testing.T) {
	var args = ProgramArgs{}

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

tests/multi2/head.h, line 5 at r2 (raw file):

#include <stdio.h>

void say_four(){

These should be just prototypes, the whole header file should be:

#include <stdio.h>

#ifndef HEADER
#define HEADER

void say_four();
void say_two();

#endif /* HEADER */

tests/multi2/main.c, line 1 at r2 (raw file):

#include"head.h"

If you are going to use the header file for forward-declaring functions you should place the actual functions after the main.

tests/multi2/main.c, line 3 at r2 (raw file):

#include"head.h"

#ifndef HEADER

*.c files should not contains these conditional pragmas because C files are not used in #include. Please remove the #if and #define lines.

tests/multi3/1.c, line 4 at r2 (raw file):

#ifndef SQLITE
#define SQLITE

Remove these lines.

tests/multi3/1.c, line 13 at r2 (raw file):

#include <stdio.h>

int main(){

I don't understand what these files are testing that is different from multi2?

tests/multi3/1.h, line 4 at r2 (raw file):

#define SQLITE

void sql(){

void sql();

tests/multi4/include/head.h, line 3 at r2 (raw file):

#include <stdio.h>

void print(){

Add the comment above:

// multi4 tests that functions can be declared in the header.

tests/trigraph/main.c, line 3 at r2 (raw file):

#include <stdio.h>
int main(){
        printf("??=");

Add the comment above:

// Trigraph tests require the `-trigraph` clang option, so it cannot be amount other general tests.

This also raised the question. You should have the same file in the tests directory to make sure the default behaviour is also the same.

Comments from Reviewable

Konstantin8105 · 2017-11-09T08:42:22Z

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

a discussion (no related file):

Previously, elliotchance (Elliot Chance) wrote…

There is a merge conflict from an earlier PR that added some stats to the PR result.

Ok.

main.go, line 328 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

"Pass arguments to clang. You may provide multiple -clang-flag items."

Done.

main_test.go, line 338 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

Done.

main_test.go, line 372 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

Done.

main_test.go, line 405 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

We must always use DefaultProgramArgs() and override all the setting you need to keep it consistent.

Done.

tests/multi2/head.h, line 5 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

These should be just prototypes, the whole header file should be:
#include <stdio.h>

#ifndef HEADER
#define HEADER

void say_four();
void say_two();

#endif /* HEADER */

Done.

tests/multi3/1.c, line 4 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Remove these lines.

No. case multi3 is prototype of SQLITE.

tests/multi3/1.c, line 13 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I don't understand what these files are testing that is different from multi2?

Main different is - using two *.c files - like in SQLITE, it is happen because if you run clang -E 1.c 2.c and you have some #define LIB in both files, then clang don't defined for both files.

tests/multi3/1.h, line 4 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

void sql();

No. It is really void sql(){};. It is not a mistake.

tests/multi4/include/head.h, line 3 at r2 (raw file):

// multi4 tests that functions can be declared in the header.
Done.

tests/trigraph/main.c, line 3 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Add the comment above:
// Trigraph tests require the `-trigraph` clang option, so it cannot be amount other general tests.
This also raised the question. You should have the same file in the tests directory to make sure the default behaviour is also the same.

Done.

Comments from Reviewable

Konstantin8105 · 2017-11-10T07:09:28Z

👍 Ready for review.
Now, compile sqlite3 is platform depend and need more expertise for understood - How to compile any group of source code.

Konstantin8105 · 2017-11-14T06:57:22Z

Review status: 3 of 19 files reviewed at latest revision, 8 unresolved discussions.

tests/multi2/main.c, line 1 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

If you are going to use the header file for forward-declaring functions you should place the actual functions after the main.

If you want, then I will also that case.

tests/multi2/main.c, line 3 at r2 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

*.c files should not contains these conditional pragmas because C files are not used in #include. Please remove the #if and #define lines.

I will add a new case with without #if and ...

Comments from Reviewable

elliotchance · 2017-11-14T23:25:53Z

Reviewed 15 of 16 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):
I'm sorry for the slow reply.

After reviewing the changes I see what's trying to be achieved but it it hard to understand with generic names like "multi2". To make it clear for the next person to maintain we need to be clear about what each of the tests so and how they differ from each other.

We can reduce all the multi files into a single folder that contains all of these cases with clear documentation:

multi/header.h:

#include <stdio>

// This header file is included by multiple C files. Make sure the preprocessor
// is working correctly by not declaring duplicates.
#ifndef HEADER
#define HEADER

// Headers usually do not contain whole functions, but we want to make sure this
// is still OK to do.
void say_four() {
    printf("4");
}

// Forward-declared prototypes that are defined in one of our other C files.
void say_two();   // main1.c
void say_three(); // main2.c

#endif /* HEADER */

multi/main1.c:

#include <stdio>
#include "header.h"

int main() {
    say_two();
    say_three();
    say_four();

    return 0;
}

// The body for the definition (declared in the header). Notice this is declared
// after using the forward declaration above.
void say_two() {
    printf("2");
}

multi/main2.c:

#include <stdio>
#include "header.h"

void say_three() {
    printf("3");
}

You will still need to keep the trigraphs.c separate.

I will dismiss all of the discussions on multi files, please create a single multi folder with the examples above.

ast/empty_decl.go, line 21 at r3 (raw file):

		Addr:       ParseAddress(groups["address"]),
		Pos:        NewPositionFromString(groups["position"]),
		Position2:  groups["position2"],

This is a position so it should be parsed like the line above.

transpiler/transpiler.go, line 379 at r4 (raw file):

	case *ast.EmptyDecl:
		// Fix that...

Fix this?

travis/test.sh, line 100 at r4 (raw file):

# sqlite3.c
echo "Transpiling sqlite3.c..."
./c2go transpile -o=$SQLITE_TEMP_FOLDER/sqlite3.go $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/sqlite3.c >> $OUTFILE 2>&1

These two transpile steps need to be combined now:

echo "Transpiling sqlite3 source files..."

./c2go transpile $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/sqlite3.c $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/shell.c >> $OUTFILE 2>&1

SQLITE_WARNINGS=`cat $SQLITE_TEMP_FOLDER/sqlite3.go | grep "// Warning" | wc -l`

Replace $SQLITE_WARNING_SQLITE3 + $SQLITE_WARNING_SHELL below with $SQLITE_WARNINGS.

travis/test.sh, line 117 at r4 (raw file):

# SQLITE
echo "----------------------"

I dont understand what all this code below here is for?

Comments from Reviewable

Konstantin8105 · 2017-11-15T20:06:21Z

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

a discussion (no related file):

Previously, elliotchance (Elliot Chance) wrote…

I'm sorry for the slow reply.

After reviewing the changes I see what's trying to be achieved but it it hard to understand with generic names like "multi2". To make it clear for the next person to maintain we need to be clear about what each of the tests so and how they differ from each other.

We can reduce all the multi files into a single folder that contains all of these cases with clear documentation:

multi/header.h:
#include <stdio>

// This header file is included by multiple C files. Make sure the preprocessor
// is working correctly by not declaring duplicates.
#ifndef HEADER
#define HEADER

// Headers usually do not contain whole functions, but we want to make sure this
// is still OK to do.
void say_four() {
    printf("4");
}

// Forward-declared prototypes that are defined in one of our other C files.
void say_two();   // main1.c
void say_three(); // main2.c

#endif /* HEADER */
multi/main1.c:
#include <stdio>
#include "header.h"

int main() {
    say_two();
    say_three();
    say_four();

    return 0;
}

// The body for the definition (declared in the header). Notice this is declared
// after using the forward declaration above.
void say_two() {
    printf("2");
}
multi/main2.c:
#include <stdio>
#include "header.h"

void say_three() {
    printf("3");
}
You will still need to keep the trigraphs.c separate.

I will dismiss all of the discussions on multi files, please create a single multi folder with the examples above.

Done.

ast/empty_decl.go, line 21 at r3 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

This is a position so it should be parsed like the line above.

Ok, so I grep little bit:

grep -rnw -e Position2 | grep -v test 
parm_var_decl.go:44:		Position2:    strings.TrimSpace(groups["position2"]),
var_decl.go:42:		Position2:    strings.TrimSpace(groups["position2"]),
indirect_field_decl.go:28:		Position2:  strings.TrimSpace(groups["position2"]),
typedef_decl.go:39:		Position2:    strings.TrimSpace(groups["position2"]),
record_decl.go:43:		Position2:  strings.TrimSpace(groups["position2"]),
function_decl.go:44:		Position2:    strings.TrimSpace(groups["position2"]),
empty_decl.go:21:		Position2:  groups["position2"],
enum_constant_decl.go:26:		Position2:  groups["position2"],
enum_decl.go:24:		Position2:  NewPositionFromString(groups["position2"]),
go_stmt.go:21:		Position2:  groups["position2"],
field_decl.go:30:		Position2:  strings.TrimSpace(groups["position2"]),

May I change on other ast elements?

transpiler/transpiler.go, line 379 at r4 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Fix this?

Add warning for future

travis/test.sh, line 100 at r4 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

These two transpile steps need to be combined now:
echo "Transpiling sqlite3 source files..."

./c2go transpile $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/sqlite3.c $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/shell.c >> $OUTFILE 2>&1

SQLITE_WARNINGS=`cat $SQLITE_TEMP_FOLDER/sqlite3.go | grep "// Warning" | wc -l`
Replace $SQLITE_WARNING_SQLITE3 + $SQLITE_WARNING_SHELL below with $SQLITE_WARNINGS.

Done, but not direct like in your snippet. Because:

./c2go transpile $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/sqlite3.c $SQLITE_TEMP_FOLDER/$SQLITE3_FILE/shell.c >> $OUTFILE 2>&1

Not acceptable for clang:

panic: clang failed: exit status 1:

/usr/include/pthread.h:743:12: warning: declaration of built-in function '__sigsetjmp' requires inclusion of the header <setjmp.h> [-Wbuiltin-requires-header]
extern int __sigsetjmp (struct __jmp_buf_tag *__env, int __savemask) __attribute__ ((__nothrow__));
           ^
/tmp/SQLITE/sqlite-amalgamation-3190300/sqlite3.c:26938:9: warning: equality comparison with extraneous parentheses [-Wparentheses-equality]
  if( (p==0) ) return 7;
       ~^~~

.......

It is happen, because clang work with multifiles like we expected. clang transpile input files separatly, so if in first file we have #ifdef ..., then definition is not continue to next files.
So, we have to continues definition by hand. In our case for sqlite that also dependence at platform(linux, darwin).

In detail:
Step 1 : file shell.c include sqlite.h - Ok.
Step 2 : file sqlite3.c include body of file sqlite.h inside! But defenition SQLITE from step 1 is not continie to here.
So, If we stay like that, then after combine of clang results from both steps, then we will have body of file sqlite.h twice.

For solving that, at step 2 we include line at begin of source sqlite3.c - #include "sqlite.h". So, defenition SQLITE inside sqlite3.c will work and don't included.

Now, after combine of clang result we will have twice including of file sqlite.h, but for preprocessor - it is ok and preprocessor not gived for transpiling twice includes.

travis/test.sh, line 117 at r4 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I dont understand what all this code below here is for?

See upper.

Comments from Reviewable

elliotchance · 2017-11-15T21:32:26Z

Reviewed 30 of 30 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

main_test.go, line 132 at r5 (raw file):

				args = append(
					args,
					//	"-race",

Please leave this turned on. I know it makes the tests slower but its important to catch race conditions if they occur because they can be almost impossible to debug.

main_test.go, line 315 at r5 (raw file):

		{
			[]string{
				"./tests/multi/case1/four.c",

As I said with my original comment there is no need to have multiple "cases" this is just confusing and makes the build slow. They are all testing the same thing.

You only need three files for a single multi file test. All of the cases can do into those file (as I had documented).

ast/empty_decl.go, line 21 at r3 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

Ok, so I grep little bit:

grep -rnw -e Position2 | grep -v test 
parm_var_decl.go:44:		Position2:    strings.TrimSpace(groups["position2"]),
var_decl.go:42:		Position2:    strings.TrimSpace(groups["position2"]),
indirect_field_decl.go:28:		Position2:  strings.TrimSpace(groups["position2"]),
typedef_decl.go:39:		Position2:    strings.TrimSpace(groups["position2"]),
record_decl.go:43:		Position2:  strings.TrimSpace(groups["position2"]),
function_decl.go:44:		Position2:    strings.TrimSpace(groups["position2"]),
empty_decl.go:21:		Position2:  groups["position2"],
enum_constant_decl.go:26:		Position2:  groups["position2"],
enum_decl.go:24:		Position2:  NewPositionFromString(groups["position2"]),
go_stmt.go:21:		Position2:  groups["position2"],
field_decl.go:30:		Position2:  strings.TrimSpace(groups["position2"]),

May I change on other ast elements?

All of the other cases can go into another PR.

travis/test.sh, line 117 at r4 (raw file):

Previously, Konstantin8105 (Konstantin) wrote…

See upper.

Remove all the code below. We only want to deal with transpiring the amalgamated files to get the warnings right now.

Comments from Reviewable

…angFlag

Konstantin8105 · 2017-11-16T11:53:32Z

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

main_test.go, line 132 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Please leave this turned on. I know it makes the tests slower but its important to catch race conditions if they occur because they can be almost impossible to debug.

Done.

main_test.go, line 315 at r5 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

As I said with my original comment there is no need to have multiple "cases" this is just confusing and makes the build slow. They are all testing the same thing.

You only need three files for a single multi file test. All of the cases can do into those file (as I had documented).

Done.

ast/empty_decl.go, line 21 at r3 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

All of the other cases can go into another PR.

Ok.

travis/test.sh, line 117 at r4 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

Remove all the code below. We only want to deal with transpiring the amalgamated files to get the warnings right now.

Done.

Comments from Reviewable

elliotchance · 2017-11-16T23:38:07Z

Reviewed 6 of 35 files at r5, 18 of 23 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Konstantin8105 added 11 commits November 8, 2017 12:16

add flags for clang

6f2ff12

add flags

c18382b

add tests

cb325cc

add tests

57a016e

add tests

5de9983

add fail test

57c590d

change tests

f2b7a03

add test like sqlite

33ee6b4

add trigraph test

ddf454b

changes intravis

6122252

cleaning

225fb29

Konstantin8105 mentioned this pull request Nov 8, 2017

Add Clang flag #331

Closed

Konstantin8105 added 2 commits November 8, 2017 22:45

add tests and platform specific of travis flag

4627de3

change MAC flag in travis

27bb47b

Konstantin8105 added 2 commits November 9, 2017 00:17

Mac support

b8f289b

Resolving

6d2c845

Konstantin8105 added 8 commits November 9, 2017 11:47

after review

eca637c

CLI fix

96bad70

add EmptyDecl

f21f5cb

minor

6eae286

code climate

6b5d884

empty

df1e088

add flag -e in echo commands

9aa65a8

fix linux

e70dfa5

Konstantin8105 mentioned this pull request Nov 14, 2017

Maintain comments in Go #121

Closed

after review

c121213

after review

7b46359

Merge branch 'master' of https://github.com/elliotchance/c2go into Cl…

769b87c

…angFlag

Konstantin8105 added 2 commits November 16, 2017 14:55

after review

e1e91e9

external header.h

edb9019

Konstantin8105 mentioned this pull request Nov 16, 2017

AST. Position2 #354

Open

Konstantin8105 added 2 commits November 16, 2017 15:18

change output

8848dbf

curl

1e83289

elliotchance changed the title ~~Clang flag~~ Send clang options with "-clang-flag". Fixes #331 Nov 16, 2017

elliotchance merged commit 5ff3d07 into elliotchance:master Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send clang options with "-clang-flag". Fixes #331 #337

Send clang options with "-clang-flag". Fixes #331 #337

Konstantin8105 commented Nov 8, 2017 •

edited by elliotchance

Loading

codecov bot commented Nov 8, 2017 •

edited

Loading

Konstantin8105 commented Nov 8, 2017

elliotchance commented Nov 9, 2017

Konstantin8105 commented Nov 9, 2017

Konstantin8105 commented Nov 10, 2017

Konstantin8105 commented Nov 14, 2017

elliotchance commented Nov 14, 2017

Konstantin8105 commented Nov 15, 2017

elliotchance commented Nov 15, 2017

Konstantin8105 commented Nov 16, 2017

elliotchance commented Nov 16, 2017

Send clang options with "-clang-flag". Fixes #331 #337

Send clang options with "-clang-flag". Fixes #331 #337

Conversation

Konstantin8105 commented Nov 8, 2017 • edited by elliotchance Loading

codecov bot commented Nov 8, 2017 • edited Loading

Codecov Report

Konstantin8105 commented Nov 8, 2017

elliotchance commented Nov 9, 2017

Konstantin8105 commented Nov 9, 2017

Konstantin8105 commented Nov 10, 2017

Konstantin8105 commented Nov 14, 2017

elliotchance commented Nov 14, 2017

Konstantin8105 commented Nov 15, 2017

elliotchance commented Nov 15, 2017

Konstantin8105 commented Nov 16, 2017

elliotchance commented Nov 16, 2017

Konstantin8105 commented Nov 8, 2017 •

edited by elliotchance

Loading

codecov bot commented Nov 8, 2017 •

edited

Loading