Re: Regarding my old mmap patch

On 9/4/06, Nuno Lopes <[email protected]> wrote:
> Just a dew thoughts about your patch:
>
> I would replace this:
> +    data->view = MapViewOfFile( data->map, FILE_MAP_READ,
> +                                (DWORD)( data->pos >> 32 ),
> +                                (DWORD)data->pos, numb );
>
> with a simple:
> +    data->view = MapViewOfFile( data->map, FILE_MAP_READ, 0, 0, 0);
>
> because you already know that pos is zero and that way you don't have to
> play with the system granularity (thus removing some code you have there and
> also reduces the structure size :P).
>
> also I don't think you need the pos struct member (it seems pretty redudant
> to me).

That would be good until you need to tidy up large files.  I'm sure
nobody wants tidy taking up huge amounts of address space with no
discernable performance difference than if it were to read in chunks.

> This block also introduces potential mem leaks if GetFileSizeEx or
> CreateFileMapping fail but not the malloc:
>
> +    if ( !fin || !GetFileSizeEx( fp, (LARGE_INTEGER*)&fin->size ) ||
> +         fin->size <= 0 )
> +    {
> +        return -1;
> +    }
> +
> +    fin->map = CreateFileMapping( fp, NULL, PAGE_READONLY, 0, 0, NULL );
> +
> +    if ( !fin->map )
> +        return -1;
>

Indeed, I uploaded a new patch to fix that.

>
> This is just a little review. I haven't tested the patch myself, but it
> looks good at first glance.
>
>
> Regards,
> Nuno
>
>
> ----- Original Message -----
> >I finally got some more time and have submitted a new patch, this time
> > with the other more controversial stuff taken out, leaving only the
> > win32 memory mapping.
> >
> > The patch works off of tonight's CVS.
> >
> > http://sourceforge.net/tracker/index.php?func=detail&aid=1551769&group_id=27659&atid=390965
> >
> > --
> > Cory Nelson
> > http://www.int64.org
>
>


-- 
Cory Nelson
http://www.int64.org

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642