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

A HACKING.adoc file with hacking advice #925

Merged
merged 11 commits into from Nov 26, 2016

Conversation

Projects
None yet
3 participants
@gasche
Member

gasche commented Nov 17, 2016

I think that the README should present the compiler distribution, but does need to contain information that is only relevant to people hacking on it, currently it's trying to do a bit of both. There are various things on how to effectively hack that are folklore knowledge and we could profitably store in this file.

Cc @johnwhitington, our precious README steward.

@Drup

This comment has been minimized.

Show comment
Hide comment
@Drup

Drup Nov 17, 2016

Contributor

Hum, #751 ?

Contributor

Drup commented Nov 17, 2016

Hum, #751 ?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 17, 2016

Member

I guess I didn't know or forgot about it, sorry. (The fact that I didn't reply to your PR suggest that it was made in the time window where I disabled notification for new PRs in a desperate and ultimately doomed attempt to spend less time doing OCaml stuff.)

My HACKING.adoc file was sitting in a branch somewhere on my computer and I wanted to have it today as I was using the precheck thing. I'm sure I discussed it on some mantis ticket somewhere but I couldn't find it again.

Can we merge our files? Are you volunteering it do it, or should I add it to my TODO list?

Member

gasche commented Nov 17, 2016

I guess I didn't know or forgot about it, sorry. (The fact that I didn't reply to your PR suggest that it was made in the time window where I disabled notification for new PRs in a desperate and ultimately doomed attempt to spend less time doing OCaml stuff.)

My HACKING.adoc file was sitting in a branch somewhere on my computer and I wanted to have it today as I was using the precheck thing. I'm sure I discussed it on some mantis ticket somewhere but I couldn't find it again.

Can we merge our files? Are you volunteering it do it, or should I add it to my TODO list?

@Drup

This comment has been minimized.

Show comment
Hide comment
@Drup

Drup Nov 17, 2016

Contributor

I remember taking some bit and pieces of your Hacking.adoc, but it was several months ago. I can take another look, but I think it's best if you just add your stuff on top of it (so that you can decide yourself how to improve things).

Contributor

Drup commented Nov 17, 2016

I remember taking some bit and pieces of your Hacking.adoc, but it was several months ago. I can take another look, but I think it's best if you just add your stuff on top of it (so that you can decide yourself how to improve things).

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 18, 2016

Member

@Drup I merged your branch and made more changes. What do you think?

Member

gasche commented Nov 18, 2016

@Drup I merged your branch and made more changes. What do you think?

@dra27

Hope some of this is helpful - up to you if you'd prefer to change it here or in a subsequent GPR!

Show outdated Hide outdated CONTRIBUTING.md
@@ -7,9 +7,17 @@ OCaml distribution. These are just guidelines, not rules, use your
best judgment and feel free to propose changes to this document itself
in a pull request.
This document assumes that you already have a patch against the
compiler distribution sources that you wish to submit to the OCaml

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

distribution compiler sources?

@dra27

dra27 Nov 19, 2016

Contributor

distribution compiler sources?

Show outdated Hide outdated CONTRIBUTING.md
compiler distribution sources that you wish to submit to the OCaml
maintainers upstream. If you are looking for a document on how to
build the compiler distribution from sources and install it, see
[INSTALL.adoc](INSTALL.adoc) instead. If you are looking for

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

See [INSTALL.adoc](INSTALL.adoc) for details on how to build the compiler distribution from sources. (shorter sentence)

@dra27

dra27 Nov 19, 2016

Contributor

See [INSTALL.adoc](INSTALL.adoc) for details on how to build the compiler distribution from sources. (shorter sentence)

Show outdated Hide outdated CONTRIBUTING.md
build the compiler distribution from sources and install it, see
[INSTALL.adoc](INSTALL.adoc) instead. If you are looking for
a document on how to hack on the compiler distribution sources, see
[HACKING.adoc](HACKING.adoc) instead.

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

See [HACKING.adoc](HACKING.adoc) for details on how to hack the compiler distribution sources. (shorter sentence; remove word "on").

Possibly "edit" rather than "hack"?

@dra27

dra27 Nov 19, 2016

Contributor

See [HACKING.adoc](HACKING.adoc) for details on how to hack the compiler distribution sources. (shorter sentence; remove word "on").

Possibly "edit" rather than "hack"?

Show outdated Hide outdated HACKING.adoc
2. Consult link:INSTALL.adoc[] for build instructions. Here is the gist of it:
+
----
opam compiler-conf configure

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

This step is presumably in some way optional - can that be mentioned?

@dra27

dra27 Nov 19, 2016

Contributor

This step is presumably in some way optional - can that be mentioned?

Show outdated Hide outdated HACKING.adoc
* The
https://github.com/ocamllabs/compiler-hacking/wiki/Things-to-work-on[OCaml
Labs compiler-hacking wiki] contains various ideas of changes to
propose, some easy, some requiring sensibly more work.

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

sensibly → realistically? Either should be before "requiring" (i.e. "realistically requiring more work")?

@dra27

dra27 Nov 19, 2016

Contributor

sensibly → realistically? Either should be before "requiring" (i.e. "realistically requiring more work")?

Show outdated Hide outdated typing/HACKING.adoc
for a different part of the language:
link:typing/typetexp.mli[Typetexp] for type expressions,
link:typing/typecore.mli[Typecore] for the core language,
link:typing/typecore.mli[Typemod] for modules,

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

Link is wrong - should be to typemod.mli

@dra27

dra27 Nov 19, 2016

Contributor

Link is wrong - should be to typemod.mli

Show outdated Hide outdated typing/HACKING.adoc
Note on dependencies between modules::
Most of the modules presented above are inter-dependent with each
other. Since OCaml prevents circular dependencies between files, the

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

Delete "with each other" (it's tautologous).
prevents → "doesn't permit"

@dra27

dra27 Nov 19, 2016

Contributor

Delete "with each other" (it's tautologous).
prevents → "doesn't permit"

Show outdated Hide outdated typing/HACKING.adoc
and finally link:typing/includeclass.mli[Includeclass] for the object
system.
Note on dependencies between modules::

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

Personally, I'd just put "Dependencies between modules"

@dra27

dra27 Nov 19, 2016

Contributor

Personally, I'd just put "Dependencies between modules"

Show outdated Hide outdated typing/HACKING.adoc
Note on dependencies between modules::
Most of the modules presented above are inter-dependent with each
other. Since OCaml prevents circular dependencies between files, the

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

prevents → "doesn't permit"

@dra27

dra27 Nov 19, 2016

Contributor

prevents → "doesn't permit"

Show outdated Hide outdated typing/HACKING.adoc
implementation uses forward declarations, implemented with references
to functions that are filled later on. An example can be seen in
link:typing/typecore.mli[Typecore.type_module], which is filled in
link:typing/typecore.mli[Typemod].

This comment has been minimized.

@dra27

dra27 Nov 19, 2016

Contributor

Shouldn't the second one be typemod.mli?

@dra27

dra27 Nov 19, 2016

Contributor

Shouldn't the second one be typemod.mli?

This comment has been minimized.

@gasche

gasche Nov 23, 2016

Member

Right, and we should rather link to the .ml files here.

@gasche

gasche Nov 23, 2016

Member

Right, and we should rather link to the .ml files here.

Drup and others added some commits Apr 23, 2016

split HACKING.adoc with parts in {parsing,typing}/HACKING.adoc
In the interest of keeping HACKING.adoc not-too-long and
general-purpose, advice on modifying specific sub-systems of the
compiler distribution should be moved to the directory of this
sub-system.

This PR also clarifies the relations between the README, INSTALL,
HACKING and CONTRIBUTING documents.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 23, 2016

Member

Thanks a lot @dra27 for the review. I acted on your comments.

I think I'll wait a bit more in case anyone has feedback, but then I'll go ahead and merge because (1) the stuff in there is already useful so why wait and (2) contributions of new content to these documents are best made by creating new PRs on top of the current state.

Member

gasche commented Nov 23, 2016

Thanks a lot @dra27 for the review. I acted on your comments.

I think I'll wait a bit more in case anyone has feedback, but then I'll go ahead and merge because (1) the stuff in there is already useful so why wait and (2) contributions of new content to these documents are best made by creating new PRs on top of the current state.

Show outdated Hide outdated HACKING.adoc
to anyone with OCaml programming experience, and helps maintainers
and other contributors. If you also submit pull requests yourself,
a good discipline is to review at least as many pull requests as you
submit.

This comment has been minimized.

@Drup

Drup Nov 23, 2016

Contributor

The rendering of this paragraph is wrong (adoc interprets the indentation as a quotation)

@Drup

Drup Nov 23, 2016

Contributor

The rendering of this paragraph is wrong (adoc interprets the indentation as a quotation)

This comment has been minimized.

@gasche

gasche Nov 23, 2016

Member

Thanks, fixed.

@gasche

gasche Nov 23, 2016

Member

Thanks, fixed.

toplevel/:: interactive system
typing/:: typechecking
utils/:: utility libraries
yacc/:: parser generator

This comment has been minimized.

@Drup

Drup Nov 23, 2016

Contributor

I think we should not list all the files/directories. Some of them are duplicated with the sections above, and some of them are really not that interesting. In the end, it's just mostly noise.

@Drup

Drup Nov 23, 2016

Contributor

I think we should not list all the files/directories. Some of them are duplicated with the sections above, and some of them are really not that interesting. In the end, it's just mostly noise.

This comment has been minimized.

@gasche

gasche Nov 26, 2016

Member

It was present in the initial README -- it comes from the very first version of this README -- and I think it's an amusing exercise. I left the list of files. It is more useful for the "small files" (which are not justified anywhere else) than for the directories indeed, so we could think of removing them, but let's not overdo it.

@gasche

gasche Nov 26, 2016

Member

It was present in the initial README -- it comes from the very first version of this README -- and I think it's an amusing exercise. I left the list of files. It is more useful for the "small files" (which are not justified anywhere else) than for the directories indeed, so we could think of removing them, but let's not overdo it.

@gasche gasche merged commit ae754ed into ocaml:trunk Nov 26, 2016

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 26, 2016

Member

I merged.

Member

gasche commented Nov 26, 2016

I merged.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #925 from gasche/HACKING.adoc
A HACKING.adoc file with hacking advice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment