Remove configuration options --disable-force-safe-string and DEFAULT_STRING=unsafe #10893
Conversation
utils/clflags.ml
Outdated
| if Config.safe_string then ref false | ||
| else ref (not Config.default_safe_string) | ||
| (* -safe-string / -unsafe-string *) | ||
| let unsafe_string = ref false (* -safe-string / -unsafe-string *) |
There was a problem hiding this comment.
why do we still have the option here?
There was a problem hiding this comment.
Good point. My reasoning was the same as for keeping Config.safe_string around but it doesn’t make much sense for this one in fact. I’ve also removed -unsafe-string from the compiler arguments at the same time
There was a problem hiding this comment.
My expectation was that the compiler could accept to parse -safe-string or -unsafe-string options, but that it should do nothing on -safe-string, and fail with an error on -unsafe-string¹. In either case we don't need a corresponding value in Config or Clflags, unless I am missing something. Am I missing something?
¹: we could have a proper error on -unsafe-string, or we could completely remove the option and have the standard "unknown option" message, but the proper error sounds nicer.
There was a problem hiding this comment.
I was thinking keeping the value in Config is useful for backward compatibility as it is used to for printing in ocamlc -config. However you are right, they might not be needed to be exposed, only kept internal to config.mlp.
I’ve removed them and removed the commit that removed -unsafe-string to keep the nicer error message.
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
2 times, most recently
from
47bba2a to
0551306
Compare
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
from
0551306 to
18f89cb
Compare
|
Removing the options makes it very confusing if someone tries to run with them - it's better to simplify it down to a simple error message - see for example the |
|
(the changes in the compiler are great!) |
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
2 times, most recently
from
bbf01df to
4100877
Compare
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
from
4100877 to
29999ec
Compare
Done. |
There was a problem hiding this comment.
I looked at this again and I like it. I commented on two parts of the PR (OCAMLPARAM parsing, and ppx cookies) where I think we should still provide/handle safe-string information. (Basically I think the compiler should behave, to external tools, as it did in previous version when safe-string was enabled.) The maintenance cost is neglectible (it's two lines of code, in addition to the configure.ac setting) and it helps people write scripts that robustly support several OCaml versions.
(I checked in particular that ocamlc -config-var safe_string should still return true, instead of failing. My understanding is that the current PR state already provides this behavior.)
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
from
3b5d30c to
c666857
Compare
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
from
c666857 to
71d25c6
Compare
kit-ty-kate
force-pushed
the
no-unsafe-string
branch
from
71d25c6 to
041225b
Compare
|
@kit-ty-kate |
|
All green now. |
There was a problem hiding this comment.
A few tweaks in configure.ac just to accept values which are still the defaults w.r.t. #10962. I don't mind if those changes get moved there instead - the main aim of this PR (removing unsafe string completely) shouldn't be held up by keeping the odd CI script more easily maintained!
configure.ac
Outdated
| AC_ARG_VAR([DEFAULT_STRING], []) | ||
| AS_IF([test x"$DEFAULT_STRING" != "x" ], | ||
| [AC_MSG_ERROR( | ||
| [The DEFAULT_STRING option variable was deleted in OCaml 5.00.])]) |
There was a problem hiding this comment.
Possibly w.r.t. the message above:
| [The DEFAULT_STRING option variable was deleted in OCaml 5.00.])]) | |
| [OCaml 5.00 only supports immutable strings.])]) |
There was a problem hiding this comment.
I changed the message to Support for mutable strings was removed in OCaml 5.00. instead. Does that look ok?
|
I don’t think configuration arguments such as I don’t think anyone need the same configuration arguments given to several compiler version at once. It makes sense to keep |
|
It is strange - and actively irritating in at least one instance - to specify an option to |
|
(another use case for |
|
@kit-ty-kate I'm eager to merge this PR, but I would weigh in the direction of going for @dra27's suggestion. @dra27 has a phenomenal amount of experience (writing and) running compiler configure scripts, so I would trust his gut feeling here. |
|
I have no issues with the request. I just need more time, my current machine does not have the autoconf 2.69 (with ubuntu’s patch) that CI uses (I only have mainline autoconf 2.71 and 2.69) so that complicates my work setup. Plus I have to remember autoconf’s syntax/behaviour once more.. ^^" |
Actually |
|
All green and ready to be reviewed once more |
I try to keep these horrible things as usable as possible 🙂 |
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Remove -safe-string for OCaml 5. See ocaml/ocaml#10893
Nice there seems to be a push to remove deprecated functions before OCaml 5.00 (e.g. #10867), here is a PR to remove the
--disable-force-safe-stringandDEFAULT_STRING=unsafecompiler options.They have been disabled by default since OCaml 4.10 and I don’t think these options are used anywhere anymore. Incidently this removes the slightest bit of complexity in the type-checker, which is always valuable.