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

Fix some globally-visible names, and add a tool for them #973

Merged
merged 1 commit into from Mar 7, 2017

Conversation

stedolan
Copy link
Contributor

PR#1956 is about externally-linked names not prefixed with caml_, which can break other software. Since that issue was created, a couple more have snuck in (e.g. invert_root in compare.c).

This commit fixes those names, and adds tools/check-symbol-names to check for others. It may be a good idea to add:

tools/check-symbol-names byterun/libcamlrun.a

as a step in the build, but I'll leave that step for someone else.

(The only remaining offending symbol (i.e. externally visible, not prefixed with "caml", and not "main") is ensure_spacetime_dot_o_is_included, whose function I don't really understand (despite the descriptive name). Does anyone know why this is necessary? @mshinwell?)

@mshinwell
Copy link
Contributor

That Spacetime variable was to ensure that a particular object file wasn't dropped by the linker, but I will have to remind myself of the details. (It's just possible that it's redundant now.)

I'm not sure I agree with the comment on Mantis about the macros etc. not causing a problem; there's been at least one report of names clashing there (presumably because people want to use the OCaml headers and another library together, which isn't unreasonable).


nm -A -P "$@" | awk '
# ignore caml_foo, camlFoo_bar, and linker-internal _foo
$2 ~ /^(caml[_A-Z]|_)/ { next }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this script worked on the Mac as well.
At the moment I don't think it will, since all of the symbols on such platform start with an underscore...

@mshinwell
Copy link
Contributor

For the moment I suggest prefixing the Spacetime variable as well.
I made a comment about the script, but the rest looks OK.

@stedolan
Copy link
Contributor Author

stedolan commented Mar 6, 2017

Namespaced spacetime variable as well. The tool now supports OS X as well.

@mshinwell
Copy link
Contributor

This is OK, I don't know why Travis failed.

@mshinwell
Copy link
Contributor

I restarted Travis and it passed.

@mshinwell mshinwell merged commit 0c61ce8 into ocaml:trunk Mar 7, 2017
@vicuna vicuna mentioned this pull request Mar 15, 2019
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
* Cleanup Map Functor Applications

- Remove unused modules
- Replace local strings maps with globally defined one

* Put string map inside project String

* Formatting

---------

Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants