Restore -vmthreads flag as an error#2312
Conversation
|
I totally agree; this will make finding older uses of vmthreads much much easier! |
|
That seems fine to me if it helps package maintainers. When do you plan to remove these completely? |
|
@diml - never, well, or perhaps when the driver is completely overhauled. I can't conceive how this particular one would ever come back into being, but in the case of |
xavierleroy
left a comment
There was a problem hiding this comment.
The intent is good but I don't understand the way it is implemented, see below. Can you explain?
driver/main.ml
Outdated
| let _strict_formats = set strict_formats | ||
| let _no_strict_formats = unset strict_formats | ||
| let _thread = set use_threads | ||
| let _vmthread = set use_vmthreads |
There was a problem hiding this comment.
Why not failing immediately when reading the -vmthread argument?
utils/clflags.ml
Outdated
| let annotations = ref false (* -annot *) | ||
| let binary_annotations = ref false (* -annot *) | ||
| and use_threads = ref false (* -thread *) | ||
| and use_vmthreads = ref false (* -vmthread *) |
There was a problem hiding this comment.
I would really like to get rid of this use_vmthreads reference.
|
Xavier Leroy (2019/03/12 16:12 +0000):
Why not failing immediately when reading the `-vmthread` argument?
I agree it should happen like this.
> @@ -68,6 +68,7 @@ let absname = ref false (* -absname *)
let annotations = ref false (* -annot *)
let binary_annotations = ref false (* -annot *)
and use_threads = ref false (* -thread *)
+and use_vmthreads = ref false (* -vmthread *)
I would really like to get rid of this `use_vmthreads` reference.
Indeed!
|
|
@xavierleroy - the "implementation" is to revert the change from #2289, so the reference is simply that which was in I don't have a particularly strong opinion, but I believe that the arguments parser is supposed to parse the arguments and then report on any semantic issues - as this GPR stands, and in present releases, (Not that I'm in favour of the myriad references in |
|
Dear David, With all due respect I think you're overengineering it :-) It is no problem that On the other hand, I feel strongly about keeping |
|
It is the first time I have overengineered something by not writing code 🙂 |
2a3695e to
6231ad9
Compare
This reverts commit 6231ad9.
xavierleroy
left a comment
There was a problem hiding this comment.
Looks fine to me. Thank you for the changes.
I'm sorry, I didn't look at #2289 in time to comment there. Apropos #2276 (comment), however unused it may be, I don't think that a CLI flag should simply disappear.
This GPR restores
-vmthreadwith an adapted version of the deprecation message as an error message and also keeps theuse_vmthreadspart of ppx contexts.I realise how pointless this looks, but experience of firefighting breakage around removing/renaming flags and things in the opam, opam-depext and various other parts of that "which no one uses" has left me highly suspicious of removing flags or variables ever!