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

Get rid of the direct call to the C preprocessor in the testsuite #8528

Merged
merged 1 commit into from Mar 21, 2019

Conversation

Projects
None yet
3 participants
@shindere
Copy link
Contributor

commented Mar 20, 2019

Cc @damiendoligez

This is a follow-up to #2311.

Call the C preprocessor through the C compiler rather than calling it
directly.

This required the definition of a new ocamltest variable,
ocaml_filetype_flag, which makes it possible to override the filetype
inferred by the compiler from the extnesion of the source file.

@shindere shindere force-pushed the shindere:use-impl-in-testsuite branch from 5ddd29c to e0e5292 Mar 20, 2019

@dra27
Copy link
Contributor

left a comment

I'm not convinced by having a .c file which is actually an ml file (even with .ml.c is nice) - if it can be solved by a cp of the original, that'd be nicer!

Show resolved Hide resolved configure.ac
@@ -1,5 +1,6 @@
(* TEST
flags = "-pp '${c_preprocessor}'"
ocaml_filetype_flag = "-impl"

This comment has been minimized.

Copy link
@dra27

dra27 Mar 20, 2019

Contributor

Having an OCaml file with a .c suffix feels really strange, however unlikely it is to be being edited/viewed in future. Could this instead work by leaving it a .ml file but copying it into the test directory as a .c file?

Either way, there should be a comment somewhere explaining why this file has to have a .c extension.

@shindere shindere force-pushed the shindere:use-impl-in-testsuite branch 3 times, most recently from 95956e7 to b3bd5c8 Mar 20, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@shindere shindere force-pushed the shindere:use-impl-in-testsuite branch from b3bd5c8 to 4dad405 Mar 20, 2019

@damiendoligez
Copy link
Member

left a comment

Looks good. The .ml.c extension is slightly annoying but I don't think we can get rid of it, nor should we spend too much time trying.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

No, it's all good - I was definitely only meaning to try to cp the file if that was a quick approach which would work, but as it's not then let's go ahead like this. It's a definite improvement not to be having to second-guess how to call the C preprocessor in configure.ac any more.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Apropos #2320 (comment), I'm not sure this needs to go in 4.08, does it? It's not fixing an actual bug I think?

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@shindere - the change to CPP (propagated to tools/config.ml) is the bigger and more visible one, so removing the confusion of DIRECT_CPP from any release I guess makes sense too.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Get rid of the direct call to the C preprocessor in the testsuite
Call the C preprocessor through the C compiler rather than calling it
directly.

This required the definition of a new ocamltest variable,
ocaml_filetype_flag, which makes it possible to override the filetype
inferred by the compiler from the extnesion of the source file.

@shindere shindere force-pushed the shindere:use-impl-in-testsuite branch from 4dad405 to 0e0ee20 Mar 21, 2019

@shindere shindere merged commit b56c4ff into ocaml:trunk Mar 21, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

shindere added a commit that referenced this pull request Mar 21, 2019

Get rid of the direct call to the C preprocessor in the testsuite (#8528
)

Call the C preprocessor through the C compiler rather than calling it
directly.

This required the definition of a new ocamltest variable,
ocaml_filetype_flag, which makes it possible to override the filetype
inferred by the compiler from the extnesion of the source file.
@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.