Skip to content

Commit

Permalink
revert #244
Browse files Browse the repository at this point in the history
The logic in this patch is wrong:
- setting the margins is not the responsibility of the color-handling code
- because setup_colors is called at inpredictable times (on the first error/warning),
  the logic makes it very difficult to correctly set margins for `{str,err}_formatter`

The patch was originally proposed in the caml-list discussion
  https://sympa.inria.fr/sympa/arc/caml-list/2015-09/msg00164.html
but it does not convincingly solve the problem:
- there is no reason to use `std_formatter`
  rather than `err_formatter` as a reference, and
- the user can set the both margins themselves anyway.

In particular, since the 4.08 changes to error/warning
representations, we don't use intermediary formatters anymore to
produce error/warning messages, so setting `Formatter.std_formatter`
directly works as expected *when* this formatter is used to print to
the toplevel (the current default, which may change in the future).

Note: We have an API in `Location` to access and configure
error/warning formatters, but it is not accessible from the
toplevel. Changing the margins without using this API is fragile.
For example, utop and expect-tests change the formatter away from the
default.
  • Loading branch information
gasche committed Dec 21, 2019
1 parent 0da925c commit 8c36e79
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
13 changes: 13 additions & 0 deletions Changes
Expand Up @@ -54,6 +54,19 @@ Working version
symbol in both the static and the dynamic symbol tables.
(Sébastien Hinderer, review by Gabriel Scherer and David Allsopp)

* #9197: remove compatibility logic from #244 that was designed to
synchronize toplevel printing margins with Format.std_formatter,
but also resulted in unpredictable/fragile changes to formatter
margins.
Setting the margins on the desired formatters should now work.
typically on `Format.std_formatter`.
Note that there currently is no robust way to do this from the
toplevel, as applications may redirect toplevel printing. In
a compiler/toplevel driver, one should instead access
`Location.formatter_for_warnings`; it is not currently exposed
to the toplevel.
(Gabriel Scherer, review by Armaël Guéneau)

### Manual and documentation:

- #9141: beginning of the ocamltest reference manual
Expand Down
2 changes: 0 additions & 2 deletions utils/misc.ml
Expand Up @@ -693,8 +693,6 @@ module Color = struct
} in
pp_set_mark_tags ppf true; (* enable tags *)
pp_set_formatter_stag_functions ppf functions';
(* also setup margins *)
pp_set_margin ppf (pp_get_margin std_formatter());
()

external isatty : out_channel -> bool = "caml_sys_isatty"
Expand Down

0 comments on commit 8c36e79

Please sign in to comment.