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

reduce boilerplate in compiler command line parsing #1941

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@sliquister
Copy link
Contributor

commented Jul 29, 2018

Changing compiler flags is quite annoying currently due to how it requires:

  1. adding a ref to Clflags, and to the mli
  2. adding the flag to main_args.ml
  3. and add the callback for said flag in the right module type (ml and mli)
  4. list said flag in the body of n functors
  5. implement said flag at the n+ call sites of these n functors, almost all of which will simply set Clflags
  6. maybe updating man pages

I propose a change such that the work required is:

  1. unchanged
  2. adding the flag to main_arg.ml, but the spec sets Clflags.foo instead of calling a f parameter
  3. gone
  4. unchanged (can be improved upon later)
  5. gone
  6. unchanged

You can see the concrete impact of that change in the second commit, for a fraction of the flags (not every flag, because such a change would conflict a lot and for some flags, such a change would require more refactoring, because of OCAMLPARAM in particular).

Now about the implementation.

The reason why the interpretation of flags is left to a functor is ocamlcp, which wants to interpret flags differently from everyone else: print them back, instead of setting Clflags. I don't think we can change ocamlcp to have a command line like ocamlcp ocamlcp-flags -- ocamlc-flags at this point (which would be much simpler).

So I change ocamlcp so it reinterprets flags at a different level: at the level of the Arg.spec, which is simpler (and less error prone: who knows if there are typos in the flag names given to ocamlc by ocamlcp?). With that, most flags (or perhaps all flags) can simply set references in Clflags.

(an alternative implementation to get rid of 5. but not 3. would have been for main_args.ml to provide a structure that implements as much as possible [val _mk_foo] as [Clflags.foo := true], but I see no point in preserving the parametrization, and the code is more direct without it)

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

So long-term what's the criterion for which command-line definitions have a f parameter, and which do not?

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

I'm not entirely sure.
The best outcome is none of them take parameters. I suppose but I'm not sure that it works for flags that print and exit.
If that's problematic, maybe a few flags stay parametrized (that would be the short term solution for some flags), or maybe new refs are added to Clflags to allow the various callers to react differently.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

FYI: in #1972 I just moved the -nopervasives from Compiler_options to Common_options.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

cc @Octachron or @nojb, who may be in good spirit for a review.

(Personally I find the lack of homogeneity in the resulting code a bit perplexing/unpleasant, but I don't want to do a full review now so I don't have a definitive opinion.)

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch from 7dcc591 to 86e53b9 Aug 7, 2018

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

@trefis that's fine, that's why I only refactored a subset of flags that were not too likely to conflict

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch from 86e53b9 to c974884 Aug 27, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2018

I like the idea of simply reinterpreting the arguments in ocamlcp. Would it not be possible to manually replace/update the handful of remaining arguments that have a different behaviors in ocamlcp.ml? Then all the complex functor scaffolding could be replaced with lists of arguments.

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Replacing all the functors with list of arguments is indeed the goal, assuming it works. And in fact, the list of arguments should probably be composed instead of repeated (let typing_args = ..;; let bytecomp_args = ..;; let ocamlc_args = List.concat [ typing_args; bytecomp_args; .. ]).

ocamlcp doesn't require any functorization, but we can't just remove the functorization by changing ocamlcp, because the implementation of OCAMLPARAM in ocamlc/ocamlopt hijacks the interpretation of the flags for its own purposes currently.

To get to the defunctorized state, we need to remove the use of functorization.
Some of it is done in this pull request.
A large fraction of the rest is easy and can be done (it wasn't done to avoid conflicts).
ocamlcp still cares about how the source files are specified (anonymous, -impl, -intf, - file), but that can be removed by (optionally) passing -impl or -intf to -pp commands ( ocamlc -pp bla -impl bla -intf bla cannot work right now because the compiler doesn't tell the preprocessor whether input files are .ml or .mli).
At this point, ocamlcp should not need the functorization anymore.
To remove the rest, OCAMLPARAM should be refactored to wrap the command line parsing instead of inserting itself in the middle of it. That's harder, as that code is very confusing.

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch from c974884 to f3b35a4 Sep 1, 2018

@@ -471,12 +471,16 @@ let mk_thread f =
" (deprecated) same as -I +threads"
;;

let mk_dtimings f =
"-dtimings", Arg.Unit f, " Print timings information for each pass";
let mk_dtimings =

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 3, 2018

Contributor

Should those argument be renamed as just dtimings to impart the fact that they are building anything right now?

This comment has been minimized.

Copy link
@sliquister

sliquister Sep 4, 2018

Author Contributor

Yeah, that sounds a bit better, I made this change.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

I am not sure if I understand the problem with OCAMLPARAM and the defunctorisation, as far I can see it just reimplements the parsing and interpretation of argument on its own?

Going back to the topic at hand, I think that the first commit can stand on its own and is already an interesting simplification; but the second commit really feels like a partial demonstration. Do you plan to complete it in this PR if the principle is approved? Or might it better to split this commit in an independent PR?

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch 2 times, most recently from 6125251 to 3fbc687 Sep 4, 2018

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

I am not sure if I understand the problem with OCAMLPARAM and the defunctorisation, as far I can see it just reimplements the parsing and interpretation of argument on its own?

If you look at the how -ppx is implemented in optmain.ml, you'll see it sets Compenv.first_ppx instead of setting Clflags.ppx as one would expect. So it differs from what ocamldoc does for instance. I think everything should just set Clflags.ppx.

Going back to the topic at hand, I think that the first commit can stand on its own and is already an interesting simplification; but the second commit really feels like a partial demonstration. Do you plan to complete it in this PR if the principle is approved? Or might it better to split this commit in an independent PR?

The first commit can definitely stand on its own. I can make a pull request with just that, if you'd merge it. The second partial is partial in that it doesn't cover all flags, but I don't know if you mean it's not enough to be sure the remaining flags can be converted, or if you simply mean not all flags are converted. I'd write the rest of the changes if this is approved in principle, but I'd rather we merge the parts that are ready rather than waiting for everything to be written. At least merging enough of the changes that reducing the functors is simply deleting code rather than also refactoring, even if reducing the functors is everything in one go (though if that is one change, then I'd like it to not linger, to reduce conflicts).

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

but I don't know if you mean it's not enough to be sure the remaining flags can be converted, or if you simply mean not all flags are converted.

Both, really: most flags can be trivially converted, but some flags would require some work; and the delimitation is not completely obvious.

Looking at the second commit, I think that I missed the internal logic behind the choice of converted flags. As far as I can see, this commit is converting all debugging flags, isn'it? I think this point should be made more obvious in the commit message. Similarly, adding some preamble in main_args.ml and maybe grouping common set of options with meaningful name would help to coalesce the converted arguments in a coherent slice of flags.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

@sliquister , what do you think of dropping the second commit in order to to move on with the core internal change?

@gasche

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

(By "dropping" I guess @Octachron suggests splitting in two PRs, one that we could review and merge soon, and one that is still considered WIP.)

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

That was a bit unclear indeed, I did mean having two PRs, one for the core changes, and one for the conversion to the new mechanism. Thanks @gasche .

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch 2 times, most recently from c3c6bb2 to a6a7e5a Jan 16, 2019

@sliquister

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

I rebased the first commit and gave up on the second one.

Show resolved Hide resolved tools/ocamlcp.ml Outdated

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch from a6a7e5a to d5fcfbc Jan 17, 2019

@Octachron
Copy link
Contributor

left a comment

This change looks good to me: it removes a lot of moving part from the construction of the arguments of ocamlcp and ocamloptp and is a good step towards a simplification of the handling of arguments in the compiler.

Show resolved Hide resolved driver/main_args.mli Outdated

@sliquister sliquister force-pushed the sliquister:ocamlcp-dedup3 branch from d5fcfbc to 427e5a5 Jan 23, 2019

@Octachron Octachron merged commit f15ee3b into ocaml:trunk Feb 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Octachron

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Merged, thanks for your patience @sliquister .

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.