[llvm-commits] [PATCH] Add mapped_file_region for read/write mapped files.

[Michael Spencer]

On Mon, Aug 6, 2012 at 2:58 PM, Nick Kledzik <kledzik at apple.com> wrote:
>> +  mapped_file_region(const Twine &path,
>> +                     mapmode mode,
>> +                     uint64_t length,
>> +                     uint64_t offset,
>> +                     error_code &ec);
>
> Something about constructors that also return an error_code sits wrong with me.  In the error case, the constructor succeeds but you are left with a half constructed object.

I don't like it either, but the alternatives don't allow RAII. And all
of them (except exceptions) require checking a value to make sure the
returned mapped_file_region is valid.

> By having the error_code at the end, you can now no longer use default values for the length and offset parameters.  Your test case has to pass 0, 0 for them which is non-obvious what is happening.  Since mapping the whole file is common, it would be nice if that case could just be called without the offset and length parameters.

It's easy to just move ec to be the first parameter.

> To support unix and Windows which have different state requirements, you had to create another struct (mapstate) to encapsulate that.  So now there are two structs to manage.

mapstate is an internal implementation detail. Users don't know
anything about mapstate.

>
>
> I would think it solve the above three issues and be cleaner to define an abstract base class and use a factory method for creation.  Then the unix and Windows implementations have their own subclasses with the ivars they each need, and the factory method (in the .inc file) just calls the constructor on the concrete subclass.  The interface in FileSystem.h would just be the abstract interface:
>
> class mapped_file_region {
> public;
>         mapped_file_region() LLVM_EQ_DELETE;
>         mapped_file_region(mapped_file_region&) LLVM_EQ_DELETE;
>         mapped_file_region &operator =(mapped_file_region&) LLVM_EQ_DELETE;
>
>         static mapped_file_region* make(const Twine &path, mapmode mode, uint64_t length=0, uint64_t offset=0);
>
>         virtual error_code   error() const;
>         virtual mapmode    flags() const;
>         virtual uint64_t        size() const;
>         virtual char            *data() const;
> };

I have two issues with this. The first is that mapped_file_region uses
RAII and move semantics to manage its lifetime, this forces users to
use delete or wrap it in an OwningPtr. The second is that there is no
reason to have virtual functions when only one implementation can ever
be used.

This also still has the problem of partially constructed objects.

- Michael Spencer

>
>> +  int flags = mode == mapped_file_region::readwrite ? MAP_SHARED : MAP_PRIVATE;
> I also found this hard to parse.  How about:
>
> int flags = ((mode == mapped_file_region::readwrite) ? MAP_SHARED : MAP_PRIVATE);

ok.

> -Nick

- Michael Spencer

>
>
> On Aug 6, 2012, at 2:00 PM, Michael Spencer wrote:
>> The attached patch adds mapped_file_region based on
>> boost::iostreams::mapped_file. This is a replacement for
>> map_file_pages/unmap_file_pages to work properly on Windows. This can
>> also be used by MemoryBuffer to finally have memory mapped files on
>> Windows.
>>
>> The reason the previous APIs do not work for Windows is that there are
>> 3 different handles that need to be kept track of to properly unmap a
>> region. For munmap there is only 1.
>>
>> - Michael Spencer
>> <0001-PathV2-Add-mapped_file_region.-Implementation-for-Wi.patch>
>