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

[Nick Kledzik]

> +  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.

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.

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.


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;
};


> +  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);

-Nick



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>