Skip to content

Install all include files, and access internals with CAML_INTERNALS#656

Merged
lefessan merged 1 commit intoocaml:trunkfrom
lefessan:2016-07-04-caml-includes
Jul 12, 2016
Merged

Install all include files, and access internals with CAML_INTERNALS#656
lefessan merged 1 commit intoocaml:trunkfrom
lefessan:2016-07-04-caml-includes

Conversation

@lefessan
Copy link
Copy Markdown
Contributor

@lefessan lefessan commented Jul 4, 2016 •
edited
Loading

Currently, OCaml only installs some of its include files, and remove some "private" sections from these public headers. This PR changes this behavior by installing all include files, but keeps them private (i.e. they will exactly define the same values as before) unless a macro CAML_INTERNALS is defined. The rationale is that specific applications might require more access to the internals of the runtime, but they should be warned of the possibility of incompatible changes by setting this macro.

To have access to internals, all C files within the distribution must also set CAML_INTERNALS.

This PR is needed for OCamlPro's memory profiler to work on an unpatched version of OCaml.

@lefessan lefessan force-pushed the 2016-07-04-caml-includes branch 2 times, most recently from 3a56445 to 35662bd Compare July 4, 2016 21:40
@bobot
Copy link
Copy Markdown
Contributor

bobot commented Jul 5, 2016 •
edited
Loading

Instead of adding CAML_ACCESS_INTERNALS to all the .c files can't you just add -DCAML_ACCESS_INTERNALS to the compilation flags?

Could you find a place in the manual to say that there is no compatibility guaranty if this flag is selected. Could you also at the same time add a warning in the documentation that there is no guaranty if someone access a symbol of the runtime through another mean than exported interface (external in C or OCaml).

@lefessan
Copy link
Copy Markdown
Contributor Author

lefessan commented Jul 5, 2016

I have personal preference to put such information ("this source relies on the OCaml internals" => #define CAML_ACCESS_INTERNALS) into the sources, instead of putting them in the build system, that is meta-layer above the code.

Could you find a place in the manual to say that there is no compatibility guaranty if this flag is selected.
Could you also at the same time add a warning in the documentation that there is no guaranty if someone access a symbol of the runtime through another mean than exported interface (external in C or OCaml).

Where is the source of the documentation ?

@lefessan lefessan force-pushed the 2016-07-04-caml-includes branch from 8f00b25 to 3660652 Compare July 5, 2016 12:19
@chambart chambart mentioned this pull request Jul 7, 2016
Merged
@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Jul 8, 2016

I agree with Fabrice, it's better to have a #define in each file.

I think the name is not quite right. What does this have to do with "access"? It should probably be CAML_INTERNAL_DEFINITIONS or even just CAML_INTERNALS.

Where is the source of the documentation ?

Duh? It's in manual/. You should probably document this feature somewhere at the end of manual/manual/cmds/intf-c.etex.

@lefessan lefessan force-pushed the 2016-07-04-caml-includes branch from d0ff974 to 539e6fd Compare July 8, 2016 21:49
@lefessan
Copy link
Copy Markdown
Contributor Author

lefessan commented Jul 8, 2016

I have switched to CAML_INTERNALS and updated the manual. Is the short paragraph that I added enough ? Since it is supposed to grant access to the "private" parts of the headers, it was not clear for me that it had to be documented in the "public" manual...

@lefessan lefessan force-pushed the 2016-07-04-caml-includes branch from 539e6fd to 63a3924 Compare July 12, 2016 15:49
@lefessan
Copy link
Copy Markdown
Contributor Author

lefessan commented Jul 12, 2016

Sounds like there are no other comments, so merging.

@lefessan lefessan closed this Jul 12, 2016
@lefessan lefessan reopened this Jul 12, 2016
@lefessan lefessan changed the title Install all include files, and access internals with CAML_ACCESS_INTE… Install all include files, and access internals with CAML_ACCESS Jul 12, 2016
@lefessan lefessan changed the title Install all include files, and access internals with CAML_ACCESS Install all include files, and access internals with CAML_INTERNALS Jul 12, 2016
@lefessan lefessan merged commit 1e7b1d9 into ocaml:trunk Jul 12, 2016
@bobot bobot mentioned this pull request Jul 19, 2016
Merged
@lefessan
Copy link
Copy Markdown
Contributor Author

lefessan commented Aug 3, 2016

@gasche
How do you see this warning ? make all in testsuite does not show it on my laptop. So I cannot know if adding #define CAML_INTERNALS in that file will fix the problem or not.

By the way, it made me notice that there is still a private section in caml/intext.h that should be replaced by #ifdef CAML_INTERNALS...

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 3, 2016

The warning shows when I build all tests with "make all", but in the middle of the list of all tests, not in the summary at the end. Otherwise just "cd tests/lib-marshal; make" shows it. But it may depend on your compiler (I have GCC 6.1.1).

So I tested and #define CAML_INTERNALS just before #include <intext.h> (with an #undef after it) silences the warning.

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 3, 2016

@lefessan I'll let you commit this along with the intext.h fix you mentioned.

@lefessan
Copy link
Copy Markdown
Contributor Author

lefessan commented Aug 3, 2016

See #737

@dra27 dra27 mentioned this pull request Nov 13, 2016
Merged
camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
@lefessan
Merge pull request ocaml#656 from lefessan/2016-07-04-caml-includes
b56a6d0 
Install all include files, and access internals with CAML_INTERNALS
@stedolan stedolan mentioned this pull request Dec 9, 2019
Closed
@gadmm gadmm mentioned this pull request Dec 26, 2019
Closed
@lefessan lefessan deleted the 2016-07-04-caml-includes branch January 24, 2021 16:10
stedolan pushed a commit to stedolan/ocaml that referenced this pull request May 24, 2022
@gretay-js
Add flags -dcfg-equivalence-check and -dcfg-invariants (ocaml#656)
aecbda8 
* Add flags -dcfg-invariants -dcfg-equivalence-check

* Turn off cfgize testing by default

Can be enabled with -dcfg-equivalence-check compiler flag
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
@saminala
Update jobs.yml (ocaml#656)
e2ca10f 
Adding Nobias Therapeutics ad for AI Pharma Engineer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lefessan @bobot @damiendoligez @gasche