Thanks! Since this is not performance critical, it seems the code could be made much simpler by using a single pass, creating the result with Buffer.add_char.

Yeah, it was one of those things that seemed like a good idea when I started writing it! Given that I've almost certainly written that function 2 or 3 times before, can I make a case for it usefully being in the standard library (and therefore preferring the two-scan but only one copy rather than single-scan, two copy version with Buffer?). I'm happy to add the relevant test cases to the test suite, if so...

It's delicate to change the standard library because there is no clear consensus on where we want to go with it, and the current peace treaty ("only the obvious functions") does not justify adding this one. Feel free to move this to utils/misc.ml, and add testsuite coverage for it.

I didn't know about that module - I'd incorrectly thought that being used in the compiler made it a potential candidate! I'll move it there and add tests in tests/utils

First of all, this isn't solving the real problem, it's just sweeping the dust under the rug.

The real problem is not \r\n in warnings, it's naked newlines in string constants, whose semantics changes when git translates \n into \r\n. Programs with variables semantics are bad news, especially when the change is invisible.

The problem was solved a few years ago for normal strings by introducing warning 29. This solution obviously doesn't apply to quoted strings. Any ideas?

Then, if you want to fix it at the warning level, I have two remarks:

• Your solution is way too complex. Just remove all \r with this function:

    let remove_cr s =
      let b = Buffer.create 80 in
      for i = 0 to String.length s - 1 do
        if s.[i] <> '\r' then Buffer.add_char b s.[i]
      done;
      Buffer.contents b

• You shouldn't apply remove_cr to all those warnings that have no chance of containing a \r, but
  use it only in the Deprecated case of message. It makes it a bit clearer where these \r might come from.

You could also simply fix the test by using a normal string as the annotation payload.

OK, I was under the impression from dra27@3985349#commitcomment-15122153 that the inclusion of the \r\n was not considered a problem. Normalising the line endings in the parsing of attributes seems more logical to me.
Fair enough for simply removal all \r - I was concerned as to whether the message is (or might be) used elsewhere where the translation might not be wanted.

that the inclusion of the \r\n was not considered a problem

I was only describing the current state.

Normalising the line endings in the parsing of attributes seems more logical to me.

The problem is for quoted strings, where we expect a byte-level correspondence between whatever is in the source code and what is kept in the AST.

Sorry, the way I phrased that was unnecessarily defensive. There are two options that "fix" the test and therefore truly sweep up the dust:

1. Declare deprecated_module.mli as needing Unix line-endings in .gitattributes (which was my original solution which opened this) - potentially good inasmuch as it records the problem in a commit
2. Change the message to be one line only. Is there a reason why it was a multiline message originally, though?

Both options dismiss this PR and hopefully expedite the merging of #366 (which is what I'm originally after...). Does that the leave the MPR as something that wants fixing at some point?

Fair enough for simply removal all \r - I was concerned as to whether the message is (or might be) used elsewhere where the translation might not be wanted.

The message is only to be used for display, so I really think the simple solution is better.

OK - it shouldn't ever be relevant here, but my thoughts/over-engineering are probably influenced by porting work in OPAM, where \r is used for its true meaning (i.e. terminal carriage return) and the simpler solution would be semantically wrong!
As you've added to it the 4.03.0 milestone, I take it you'd like the message fixed as requested, rather than altering the test suite to "mask" the issue instead?

By adding it to the 4.03 milestone, I just mean I think it should/will be resolved before 4.03.0. I agree altering the test suite is not a satisfactory solution, and I won't fight for the simple version of CR removal.

I'll merge this PR as soon as you:

1. move normalise_eol to misc.ml.
2. add a comment to Warnings.print to explain why we need to call normalise_eol.

Happy New Year! Changes made. I apologise for creating the appearance of a fight - it's definitely your project (which I don't mean in the double-meaning that various guides as to what the British say vs. what they mean might suggest!) - I was going to put remove_cr in as suggested, which I'm quite happy to do... the conversation reveals that I should learn to put more comments as to why I've done something, as opposed to what, in my patches (which I've now done both in utils/misc.mli and also before the call to Misc.normalise_eol in utils/warnings.ml

There are more important topics, but what about doing the simpler version of removing all \r characters, but only as a rewriting for the payload string of the deprecated attribute?

Rebased with a second commit to show the changes - up to you if you would like it squashed or merged as two.

@dra27 thanks for putting up with our nitpicking :-)