Hi Levente,<br><br><div class="gmail_quote">On Tue, May 17, 2011 at 7:14 AM, Levente Uzonyi <span dir="ltr"><<a href="mailto:leves@elte.hu">leves@elte.hu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br>
On Tue, 17 May 2011, Igor Stasenko wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 17 May 2011 13:50, Levente Uzonyi <<a href="mailto:leves@elte.hu" target="_blank">leves@elte.hu</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Tue, 17 May 2011, Igor Stasenko wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 16 May 2011 20:21, Levente Uzonyi <<a href="mailto:leves@elte.hu" target="_blank">leves@elte.hu</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Mon, 16 May 2011, <a href="mailto:squeak-dev-noreply@lists.squeakfoundation.org" target="_blank">squeak-dev-noreply@lists.squeakfoundation.org</a> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Igor Stasenko uploaded a new version of CMakeVMMaker to project VM<br>
Maker:<br>
<a href="http://www.squeaksource.com/VMMaker/CMakeVMMaker-StasenkoIgor.104.mcz" target="_blank">http://www.squeaksource.com/VMMaker/CMakeVMMaker-StasenkoIgor.104.mcz</a><br>
<br>
==================== Summary ====================<br>
<br>
Name: CMakeVMMaker-StasenkoIgor.104<br>
Author: StasenkoIgor<br>
Time: 16 May 2011, 10:33:56 am<br>
UUID: 68959e6f-18b1-304c-a3de-2b84a1f2c93b<br>
Ancestors: CMakeVMMaker-IgorStasenko.103<br>
<br>
workaround the MultiByteCrapStream, which converting line endings twice<br>
on<br>
windows.<br>
</blockquote>
<br>
It's not crap, just broken.<br>
</blockquote>
<br>
Well, i don't know better wording for seeing the mess in this<br>
implementation. It contains too much bells and whistles and therefore<br>
its very easy to break it.<br>
</blockquote>
<br>
Tests can greatly decrease the chance of breaking it.<br>
<br>
</blockquote>
<br>
yes, i submitted a test yesterday.<br>
<a href="http://code.google.com/p/pharo/issues/detail?id=4236" target="_blank">http://code.google.com/p/pharo/issues/detail?id=4236</a><br>
<br>
it written in a way to reproduce an issue when running on any system<br>
(not just Windows).<br>
</blockquote>
<br></div></div>
I also have some tests which cover more cases.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I don't understand why it patching the UTF8Converter and turns it into<br>
line-ending converter..<br>
as to me, UTF8Converter have nothing to do with line ends, and such<br>
abuse of text converters should be strictly prohibited.<br>
</blockquote>
<br>
The reason is performance, and the implementation is transparent. It also<br>
simplifies MultiByteFileStream.<br>
<br>
</blockquote>
<br>
what about clarity? The problem is that this code is so optimized that<br>
it is no longer transparent. It is opaque for my eyes.<br>
I cannot fix it, because it looks very brittle and it is hard to get<br>
what it does, with all those branches, ifNotNils etc etc in almost<br>
every method there.<br>
</blockquote>
<br></div>
MultiByteFileStream is not clean yet, but it will be.<div><div></div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
And i cannot be sure if my fix won't break something else, because<br>
there is no clear separation of concerns:<br>
<br>
- use UTF8Converter for utf8 conversion and NOTHING else.<br>
- and let stream to filter input and replace cr character(s) to<br>
whatever it wants them to be<br>
<br>
<br>
And what about simplification?<br>
<br>
Do you think this is how a _simple_ method should look like?<br>
<br>
nextPut: aCharacter<br>
aCharacter isInteger<br>
ifTrue: [ ^ super nextPut: aCharacter ].<br>
(wantsLineEndConversion == true and: [ lineEndConvention notNil ])<br>
"#doConversion is inlined here"<br>
ifTrue: [<br>
aCharacter = Cr<br>
ifTrue: [ converter nextPutAll: (LineEndStrings at:<br>
lineEndConvention) toStream: self ]<br>
ifFalse: [ converter nextPut: aCharacter toStream: self ].<br>
^aCharacter ].<br>
^ self converter nextPut: aCharacter toStream: self<br>
<br>
4 branches.. while i, by following your premise, would expect a single<br>
line here:<br>
<br>
nextPut: aCharacter<br>
^ converter nextPut: aCharacter toStream: self<br>
<br>
because if you decided to delegate line ends conversion to converter<br>
(for whatever reason) , then you don't have to do anything outside of<br>
it.<br>
</blockquote>
<br></div></div>
Exactly. What I also have in my mind is to remove the #isBinary checks from the TextConverters and put them back to MultiByteFileStream, where they belong to IMHO.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'll integrate the fix for it into Squeak soon.<br>
If you're impatient you can fix it yourself, or use something else.<br>
<br>
</blockquote>
<br>
Well, i can't use something else, because its VMMaker which using<br>
CrLfFileStream for producing source files output.<br>
Maybe it worth to just use FileStream in binary mode and deal with<br>
line ends internally, to not leave chance for<br>
clever text streams to interfere.<br>
</blockquote>
<br>
CrLfFileStream is not used, since Squeak 3.x. References to it should be<br>
rewritten to use FileStream instead. Using binary mode to work around a bug<br>
is bad idea, fixing the bug is the way to go.<br>
<br>
</blockquote>
<br>
But why you stating false statements? CrLfFileStream is used, at least<br>
by VMMaker.<br>
And i was never took a deep look on a file streams to clearly say if<br>
it still used or can be removed.<br>
</blockquote>
<br></div>
It's not a false statement, print it: CrLfFileStream new. In Squeak CrLfFileStream is deprecated and if I'm not mistaken the same is true in Pharo.</blockquote><div><br></div><div>I'm just trying to eliminate the explicit references to CrLfFileStream in the Cog VMMaker, but I've hit a snag. e.g. there's this idiom:</div>
<div><br></div><div>InterpreterPlugin class methods for translation</div><div>storeString: s onFileNamed: fileName</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>"Store the given string in a file of the given name."</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>| f |</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>f := CrLfFileStream forceNewFileNamed: fileName.</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>f nextPutAll: s.</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>f close.</div><div><br></div><div>that I'd like to replace with</div><div><br></div><div>VMMaker class methods for utilities</div><div><div>newFileStream</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>"Always output files in unix lf format.</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>A single format is friendlier to e.g. external version control systems.</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>The Microsoft and old MacOS classic C compilers all accept lf format files."</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>^MultiByteFileStream new</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>lineEndConvention: #lf;</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>yourself</div></div><div><br></div><div><div>InterpreterPlugin class methods for translation</div>
<div>storeString: s onFileNamed: fileName</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>"Store the given string in a file of the given name."</div><div><br></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>| f |</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span>f := VMMaker newFileStream forceNewFileNamed: fileName.</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>f nextPutAll: s.</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span>f close.</div></div><div><br></div><div>but only class-side methods understand forceNewFileNamed:, and hence one would have to write</div><div><br></div>
<div><br></div><div><div><br></div><div><div>InterpreterPlugin class methods for translation</div><div>storeString: s onFileNamed: fileName</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>"Store the given string in a file of the given name."</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>| f |</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>f := MultiByteFileStream forceNewFileNamed: fileName.</div>
<span class="Apple-style-span" style="white-space: pre; "> f </span>lineEndConvention: #lf.<div><span class="Apple-tab-span" style="white-space: pre; "> </span>f nextPutAll: s.</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>f close</div>
</div></div><div><br></div><div>which is a tad tedious and distributed. I could have a set of convenience methods in VMMaker class, forceNewFileStream: oldFileStream etc. There could be subclasses of MultiByteFileStream purely for instance creation (MultiByteLfFileStream et al). Or...? Any thoughts?</div>
<div><br></div><div>best,</div><div>Eliot</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
So, can you clarify, can we deprecate and then completely remove<br>
CrLfFileStream from image<br>
and fix all references to it with FileStream instead?<br>
Because i like putting useless cra.. (err stuff) into trash bin.<br>
</blockquote>
<br></div>
See above.<br><font color="#888888">
<br>
<br>
Levente</font><div><div></div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But then again, it means time, which i haven't because i was focused<br>
on completely different problem.<br>
</blockquote>
<br>
I'll upload my fix to Squeak Trunk this week. Applying the changes to Pharo<br>
probably won't work out of the box, because IIRC some issues were solved<br>
differently in the past.<br>
<br>
</blockquote>
<br>
i'm not sure.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Levente<br>
<br>
</blockquote>
<br>
<br>
<br>
-- <br>
Best regards,<br>
Igor Stasenko AKA sig.<br>
<br>
</blockquote>
</div></div></blockquote></div><br>