Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:41326
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain prohost.org from 66.249.82.233 cause and error)
Cc: "Lars Strojny" <lstrojny@php.net>,
 internals@lists.php.net
Message-ID: <A7B48D5E-1E75-40BF-A04C-9A1509D7C6BD@prohost.org>
To: "John Mertic" <jmertic@php.net>
In-Reply-To: <2a9adcf0810221326u798498bbgb4ab37f7c27f21ef@mail.gmail.com>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v929.2)
Date: Wed, 22 Oct 2008 16:33:47 -0400
References: <2a9adcf0810221003i70f37558ob5777b5603865cdf@mail.gmail.com> <1224696974.17075.1.camel@localhost> <2a9adcf0810221135h1e6ed4ecub55318cfb26b3e7e@mail.gmail.com> <370571A4-8A89-412C-A8CD-1D7439C761F1@prohost.org> <2a9adcf0810221326u798498bbgb4ab37f7c27f21ef@mail.gmail.com>
Subject: Re: [PHP-DEV] [PATCH] Bug #46367 - fputcsv does not add the correct newline character on Windows
From: ilia@prohost.org (Ilia Alshanetsky)

smart_str_appendl() would be faster since it'll avoid having to  
calculate the string length, and sizeof() can be used to determine the  
length of the constant, resulting in faster code.

On a general note, I am not sure its a good idea to change the EOL for  
CSV file, since many scanners expect "\n" as a line separator and the  
change may introduce regressions.


On 22-Oct-08, at 4:26 PM, John Mertic wrote:

> Hi Ilia,
>
> Sorry for my C confusion, like I said it's been awhile.
>
> Anyways, would smart_str_appends() be the correct function to use  
> then?
>
> John Mertic
> jmertic@gmail.com
> http://jmertic.wordpress.com
>
> "Explaining a joke is like dissecting a frog: you understand it
> better, but the frog dies
> in the process." --Mark Twain
>
>
>
> On Wed, Oct 22, 2008 at 4:16 PM, Ilia Alshanetsky <ilia@prohost.org>  
> wrote:
>> You cannot use smart_str_appendc() in this case, since the EOL  
>> could be a 2
>> byte string "\r\n".
>>
>> smart_str_appendl(&csvline, PHP_EOL, sizeof(PHP_EOL)-1); should be  
>> used
>> here.
>>
>> On 22-Oct-08, at 2:35 PM, John Mertic wrote:
>>
>>> Hi Lars,
>>>
>>> Thanks for the pointers, updated the patch and added a test.
>>>
>>> Index: file.c
>>> ===================================================================
>>> RCS file: /repository/php-src/ext/standard/file.c,v
>>> retrieving revision 1.530
>>> diff -u -r1.530 file.c
>>> --- file.c      21 Oct 2008 22:06:48 -0000      1.530
>>> +++ file.c      22 Oct 2008 18:21:10 -0000
>>> @@ -2104,7 +2104,7 @@
>>>               }
>>>       }
>>>
>>> -       smart_str_appendc(&csvline, '\n');
>>> +       smart_str_appendc(&csvline, PHP_EOL);
>>>       smart_str_0(&csvline);
>>>
>>>       ret = php_stream_write(stream, csvline.c, csvline.len);
>>>
>>>
>>> John Mertic
>>> jmertic@gmail.com
>>> http://jmertic.wordpress.com
>>>
>>> "Explaining a joke is like dissecting a frog: you understand it
>>> better, but the frog dies
>>> in the process." --Mark Twain
>>>
>>>
>>>
>>> On Wed, Oct 22, 2008 at 10:36 AM, Lars Strojny <lstrojny@php.net>  
>>> wrote:
>>>>
>>>> Hi John,
>>>>
>>>>
>>>> Am Mittwoch, den 22.10.2008, 10:03 -0700 schrieb John Mertic:
>>>> [...]
>>>>>
>>>>> Below is a patch to fix this issue; it uses the constant PHP_EOL  
>>>>> to
>>>>> get the correct newline to use on the current platform:
>>>>
>>>> Thanks for your patch. A few things to mention, as it is your first
>>>> patch: please use "diff -ru" to create unified diffs. Also we try  
>>>> to
>>>> always add tests for the things we fix or create. Would you mind
>>>> creating a test for the fix you sent to make sure no regression  
>>>> happens
>>>> in the next n years?
>>>>
>>>> Thanks, Lars
>>>> --
>>>> Jabber: lars@strojny.net
>>>> Weblog: http://usrportage.de
>>>>
>>>
>>> --
>>> PHP Internals - PHP Runtime Development Mailing List
>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>> Ilia Alshanetsky
>>
>>
>>
>>
>>

Ilia Alshanetsky