You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@corinthia.apache.org by jan i <ja...@apache.org> on 2015/01/01 11:20:19 UTC
Request for review: branch RTC_platform
Hi.
platform/src/wrapper.c
platform/headers/DFPlatform.h
core/src/lib/DFZipFile.c
need a review before committing. We have API changes (minizip is isolated)
and before committing I would like to hear others opinion.
I have (based on a good hint from dennis) created origin/RTC_platform with
all the changes. Feel free to make changes directly there or discuss it
here.
I have on purpose limited the zip API to only handle one zip file....If we
one day decide to go multithreaded (in contradiction to multi process) we
need to make the variabels in wrapper.c thread_local (which is easy).
Thanks in advance for comments.
rgds
jan i
Re: Request for review: branch RTC_platform
Posted by Peter Kelly <pm...@apache.org>.
Looks good :)
When you merge into master, this is where a —squash would be appropriate - it only needs to be one commit in the history. I do this all the time with lots of temporary “work” commits and squash them all with a nice message.
—
Dr Peter M. Kelly
pmkelly@apache.org
PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
(fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
> On 1 Jan 2015, at 8:19 pm, jan i <ja...@apache.org> wrote:
>
> On 1 January 2015 at 13:25, Peter Kelly <pm...@apache.org> wrote:
>
>>> On 1 Jan 2015, at 7:11 pm, jan i <ja...@apache.org> wrote:
>>>
>>> just pushed a version that allows multiple zip files open. Remark cannot
>>> have multiple files open inside a zip file, seems to be a limitation in
>>> minizip.
>>>
>>> looking forward for comments.
>>
>> Looks good, I would just suggest dynamically allocating the myZipStorage
>> objects using malloc/free - that way we don’t have any arbitrary limit on
>> the number of files that can be open at any one time. Conceptually they’re
>> just objects like anything else; it seems it would be appropriate to define
>> a public type DFExtZipHandle or similar just like we do for any type of
>> object; you then get type checking for free from the compiler.
>>
>
> your wish just came through, just pushed that change as well.
>
> I will update master a bit later and clean origin for the RTC_platform
> branch.
>
> rgds
> jan i
>
>
>>
>> —
>> Dr Peter M. Kelly
>> pmkelly@apache.org
>>
>> PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
>> (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
>>
>>
Re: Request for review: branch RTC_platform
Posted by jan i <ja...@apache.org>.
On 1 January 2015 at 13:25, Peter Kelly <pm...@apache.org> wrote:
> > On 1 Jan 2015, at 7:11 pm, jan i <ja...@apache.org> wrote:
> >
> > just pushed a version that allows multiple zip files open. Remark cannot
> > have multiple files open inside a zip file, seems to be a limitation in
> > minizip.
> >
> > looking forward for comments.
>
> Looks good, I would just suggest dynamically allocating the myZipStorage
> objects using malloc/free - that way we don’t have any arbitrary limit on
> the number of files that can be open at any one time. Conceptually they’re
> just objects like anything else; it seems it would be appropriate to define
> a public type DFExtZipHandle or similar just like we do for any type of
> object; you then get type checking for free from the compiler.
>
your wish just came through, just pushed that change as well.
I will update master a bit later and clean origin for the RTC_platform
branch.
rgds
jan i
>
> —
> Dr Peter M. Kelly
> pmkelly@apache.org
>
> PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
> (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
>
>
Re: Request for review: branch RTC_platform
Posted by Peter Kelly <pm...@apache.org>.
> On 1 Jan 2015, at 7:11 pm, jan i <ja...@apache.org> wrote:
>
> just pushed a version that allows multiple zip files open. Remark cannot
> have multiple files open inside a zip file, seems to be a limitation in
> minizip.
>
> looking forward for comments.
Looks good, I would just suggest dynamically allocating the myZipStorage objects using malloc/free - that way we don’t have any arbitrary limit on the number of files that can be open at any one time. Conceptually they’re just objects like anything else; it seems it would be appropriate to define a public type DFExtZipHandle or similar just like we do for any type of object; you then get type checking for free from the compiler.
—
Dr Peter M. Kelly
pmkelly@apache.org
PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
(fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
Re: Request for review: branch RTC_platform
Posted by jan i <ja...@apache.org>.
On 1 January 2015 at 12:00, jan i <ja...@apache.org> wrote:
>
>
> On Thursday, January 1, 2015, Peter Kelly <pm...@apache.org> wrote:
>
>> > On 1 Jan 2015, at 5:20 pm, jan i <ja...@apache.org> wrote:
>> >
>> > Hi.
>> >
>> > platform/src/wrapper.c
>> > platform/headers/DFPlatform.h
>> > core/src/lib/DFZipFile.c
>> >
>> > need a review before committing. We have API changes (minizip is
>> isolated)
>> > and before committing I would like to hear others opinion.
>> >
>> > I have (based on a good hint from dennis) created origin/RTC_platform
>> with
>> > all the changes. Feel free to make changes directly there or discuss it
>> > here.
>>
>> I don’t understand the rationale behind this - DFZipFile already provided
>> an abstraction layer over minizip (in the sense that this is the only file
>> that used any minizip data types/functions).
>
>
> the rationale is, as discussed earlier, to isolate minizip in platform. I
> looked at moving DFZipFile.c to platform, but that would mean that
> DFstorage should also move which would be incorrect.
>
> We wanted to make platform an abstraction over OS as well as zip and other
> libraries, so we easier could replace them.
>
>>
>> > I have on purpose limited the zip API to only handle one zip file....If
>> we
>> > one day decide to go multithreaded (in contradiction to multi process)
>> we
>> > need to make the variabels in wrapper.c thread_local (which is easy).
>>
>> It needs to be multithreaded - UX Write already depends on this and the
>> old API worked fine - in fact that’s the whole reason DFZipeFile exists.
>> The minizip API already provides an handle object with which you can have
>> multiple zip files open concurrently (in the same or different threads),
>> and if we’re going to replicate the minizip API we should at minimum keep
>> this capability.
>
> ok I will ad that capability (it is quite easy to do).
>
>>
>> An example of where you might want two zip files open at the same time is
>> if you’re converting from .docx to .odt. While the current implementation
>> reads the whole source file into memory, an optimisation that would be very
>> useful for files containing large images is to have it only read the
>> contents on demands. So during a conversion you would have an input zip
>> file handle and an output zip file handle. Thread-locals will not solve
>> this problem.
>
>
>
just pushed a version that allows multiple zip files open. Remark cannot
have multiple files open inside a zip file, seems to be a limitation in
minizip.
looking forward for comments.
rgds
jan i
> rgds
> jan i
>
>>
>> —
>> Dr Peter M. Kelly
>> pmkelly@apache.org
>>
>> PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key
>> >
>> (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
>>
>>
>
> --
> Sent from My iPad, sorry for any misspellings.
>
Re: Request for review: branch RTC_platform
Posted by jan i <ja...@apache.org>.
On Thursday, January 1, 2015, Peter Kelly <pm...@apache.org> wrote:
> > On 1 Jan 2015, at 5:20 pm, jan i <jani@apache.org <javascript:;>> wrote:
> >
> > Hi.
> >
> > platform/src/wrapper.c
> > platform/headers/DFPlatform.h
> > core/src/lib/DFZipFile.c
> >
> > need a review before committing. We have API changes (minizip is
> isolated)
> > and before committing I would like to hear others opinion.
> >
> > I have (based on a good hint from dennis) created origin/RTC_platform
> with
> > all the changes. Feel free to make changes directly there or discuss it
> > here.
>
> I don’t understand the rationale behind this - DFZipFile already provided
> an abstraction layer over minizip (in the sense that this is the only file
> that used any minizip data types/functions).
the rationale is, as discussed earlier, to isolate minizip in platform. I
looked at moving DFZipFile.c to platform, but that would mean that
DFstorage should also move which would be incorrect.
We wanted to make platform an abstraction over OS as well as zip and other
libraries, so we easier could replace them.
>
> > I have on purpose limited the zip API to only handle one zip file....If
> we
> > one day decide to go multithreaded (in contradiction to multi process) we
> > need to make the variabels in wrapper.c thread_local (which is easy).
>
> It needs to be multithreaded - UX Write already depends on this and the
> old API worked fine - in fact that’s the whole reason DFZipeFile exists.
> The minizip API already provides an handle object with which you can have
> multiple zip files open concurrently (in the same or different threads),
> and if we’re going to replicate the minizip API we should at minimum keep
> this capability.
ok I will ad that capability (it is quite easy to do).
>
> An example of where you might want two zip files open at the same time is
> if you’re converting from .docx to .odt. While the current implementation
> reads the whole source file into memory, an optimisation that would be very
> useful for files containing large images is to have it only read the
> contents on demands. So during a conversion you would have an input zip
> file handle and an output zip file handle. Thread-locals will not solve
> this problem.
you are right, multizip API will be available very soon.
rgds
jan i
>
> —
> Dr Peter M. Kelly
> pmkelly@apache.org <javascript:;>
>
> PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
> (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
>
>
--
Sent from My iPad, sorry for any misspellings.
Re: Request for review: branch RTC_platform
Posted by Peter Kelly <pm...@apache.org>.
> On 1 Jan 2015, at 5:20 pm, jan i <ja...@apache.org> wrote:
>
> Hi.
>
> platform/src/wrapper.c
> platform/headers/DFPlatform.h
> core/src/lib/DFZipFile.c
>
> need a review before committing. We have API changes (minizip is isolated)
> and before committing I would like to hear others opinion.
>
> I have (based on a good hint from dennis) created origin/RTC_platform with
> all the changes. Feel free to make changes directly there or discuss it
> here.
I don’t understand the rationale behind this - DFZipFile already provided an abstraction layer over minizip (in the sense that this is the only file that used any minizip data types/functions).
> I have on purpose limited the zip API to only handle one zip file....If we
> one day decide to go multithreaded (in contradiction to multi process) we
> need to make the variabels in wrapper.c thread_local (which is easy).
It needs to be multithreaded - UX Write already depends on this and the old API worked fine - in fact that’s the whole reason DFZipeFile exists. The minizip API already provides an handle object with which you can have multiple zip files open concurrently (in the same or different threads), and if we’re going to replicate the minizip API we should at minimum keep this capability.
An example of where you might want two zip files open at the same time is if you’re converting from .docx to .odt. While the current implementation reads the whole source file into memory, an optimisation that would be very useful for files containing large images is to have it only read the contents on demands. So during a conversion you would have an input zip file handle and an output zip file handle. Thread-locals will not solve this problem.
—
Dr Peter M. Kelly
pmkelly@apache.org
PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
(fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)