Add new SerializeAs option for TerminatedSizedString.

[bevanweiss]

This allows to keep the existing TerminatedString behaviour when a field length constraint is applied, where it essentially becomes a SizedString. Whilst also allowing new behaviour that would keep the terminated string aspect, but truncate / pad the terminated string around the length constraint

Test case shows non-standard terminator (\n or 0x0A), and non-standard padding byte 0x0D just to prove that these work as expected also.

Possibly fixes #171 (confirmation required)

[jefffhaynes]

I'm not completely sold on this. Is this because you're worried about the other approach being a breaking change? It would only be a breaking change in a very specific (invalid?) use case so I'm tempted to just live with it.

[bevanweiss]

I'm not completely sold on this. Is this because you're worried about the other approach being a breaking change? It would only be a breaking change in a very specific (invalid?) use case so I'm tempted to just live with it.

I’ll recheck, but from memory, changing the TerminatedString behaviour caused test cases to fail. I think they were labelled with S7 or such. Which makes me think the existing behaviour may indeed be desirable in some situation (or potentially just a test case that is a bit too ‘aggressive’).

[bevanweiss]

I'm not completely sold on this. Is this because you're worried about the other approach being a breaking change? It would only be a breaking change in a very specific (invalid?) use case so I'm tempted to just live with it.

I’ll recheck, but from memory, changing the TerminatedString behaviour caused test cases to fail. I think they were labelled with S7 or such. Which makes me think the existing behaviour may indeed be desirable in some situation (or potentially just a test case that is a bit too ‘aggressive’).

Found the failing case (well I have quite a few currently failing... which likely reveals some bugs in this naive PR solution also)

RoundtripString
   Source: Issue34Tests.cs line 9

  Message: 
    Assert.AreEqual failed. Expected:<7>. Actual:<8>. 
  Stack Trace: 
    TestBase.Roundtrip[T](T o, Int64 expectedLength) line 51
    Issue34Tests.RoundtripString() line 12
  Standard Output: 
    Debug Trace:
    S-BinarySerialization.Test.Issues.Issue34.S7String
        S-Start: MaxLength @ 0
        S-End: MaxLength (6) @ 1
        S-Start: Value @ 1
            S-Start: Length @ 1
            S-End: Length (6) @ 2
            S-Start: Value @ 2
            S-End: Value (hello) @ 8
        S-End: Value (BinarySerialization.Test.Issues.Issue34.InternalS7String) @ 8

var expected = new S7String {Value = new InternalS7String("hello")};
var actual = Roundtrip(expected, 7);

InternalS7String is defined as a Length field (byte), and a Value field (string) with FieldLength binding. Since there's no SerializeAs( SizedString ) on the Value field, then it defaults to Terminated. This then makes the string length 6 (5+null terminator), then +1 for the length, and +1 for a MaxLength field (giving total of 8), but the test case appears to expect the string will NOT have the terminator (because of the length binding) and hence the total would be 7 (5 + 1 + 1).

Having a 'new' string type serialization, so that all existing uses can remain as they are, seems easiest still.

There's 18 failing tests when I replaced the TerminatedString implementation with my TerminatedSizedString. Including Cereal tests which threw exceptions on Array.Resize. I think the Cereal issues I'll need to investigate and fix up, since that does suggest a real bug in my implementation. It shouldn't throw such buried exceptions. The other 16 tests may be over specified, or may represent situations where this change would be breaking to uses in the real world (or may indicate more bugs in my implementation).

[jefffhaynes]

Ok, fair enough. Is this all set to go then?

[bevanweiss]

I’m not fond of ‘the feel’ of having another SerializeAs Type enum, as I’ve currently implemented it. I would still like it to be a param against the terminatedstring. I’ll have another look at it on the weekend.