Conversation
programs/fileio.c
Outdated
| static void FIO_munmap(void* buffer, size_t bufferSize) { | ||
| (void)bufferSize; | ||
| free(buffer); | ||
| static void FIO_munmap(const FIO_Dict_t* dict) { |
There was a problem hiding this comment.
same comment here, prefer FIO_Dict_t* dict as argument type.
programs/fileio.c
Outdated
| } | ||
| /* We might want to also do mapping for windows */ | ||
| static size_t FIO_createDictBufferMMap(void** bufferPtr, const char* fileName, FIO_prefs_t* const prefs, stat_t* dictFileStat) | ||
| static size_t FIO_createDictBufferMMap(FIO_Dict_t* dict, const char* fileName, FIO_prefs_t* const prefs, stat_t* dictFileStat) |
There was a problem hiding this comment.
discussion on naming convention :
When I read a function name like PREFIX_createType(), I expect it to return a Type* pointer object.
That's not the case here.
On a related note, if the object is allocated beforehand (on stack for example), I would consider PREFIX_initType(Type* v, ...) for the function name.
I note that this situation is slightly different : you want to set member dict->dictBuffer with the outcome of mmap (and I presume FIO_Dict_t is larger than this member), and return the underlying file size.
In which case, maybe FIO_setDictBufferMMap() ?
daniellerozenblit
force-pushed
the
windows-mmap-dictionaries
branch
2 times, most recently
from
45f8a1c to
7467668
Compare
…jects in free functions
daniellerozenblit
force-pushed
the
windows-mmap-dictionaries
branch
from
7467668 to
ffc7bcf
Compare
programs/fileio.c
Outdated
| ress.dict.dictBufferSize = FIO_createDictBufferMMap(&ress.dict.dictBuffer, dictFileName, prefs, &ress.dictFileStat); | ||
| } | ||
| dictBufferType = (useMMap && !forceNoUseMMap) ? FIO_mmapDict : FIO_mallocDict; | ||
| ress.dict.dictBufferSize = FIO_initDict(&ress.dict, dictFileName, prefs, &ress.dictFileStat, dictBufferType); /* works with dictFileName==NULL */ |
There was a problem hiding this comment.
The return value of FIO_initDict() is used to initialize ress.dict.dictBufferSize.
Since the stated goal of FIO_initDict() is to initialize ress.dict,
I would rather expect FIO_initDict() to directly set ress.dict.dictBufferSize as part of the initialization.
FIO_initDict() should rather return a success/error code.
…ze of the set buffer
Cyan4973
left a comment
Cyan4973
left a comment
There was a problem hiding this comment.
Looks good !
Please squash commits before merging
3 participants
This PR is a follow-up to #3486. It adds memory-mapped dictionaries for Windows.