Conversation
|
Thanks a lot for submitting the patch. I'm neither the author nor familiar with this code, but while we wait for the author to respond: do you perhaps have some minimal example demonstrating the problem that the patch is trying to address? Ideally we would probably want to add some unit tests. It's of course not your job to write unit tests. It would however greatly help to have some more information that would be sufficient to add some unit tests. |
paweljackowski
left a comment
paweljackowski
left a comment
There was a problem hiding this comment.
Thank you for your patch. I wrote pplib for luatex, I doubt it is used anywhere else. So I see no obstackles for such changes iff luatex team finds it usefull. If luatex team would like to have it in luatex, then I'd vote for merging this change to pplib master (with respect to some non-critical code comments I left)
| return NULL; | ||
| wmode = get_wstring_from_mbstring(cp, mode, wmode=NULL); | ||
| if (wmode == NULL) | ||
| return NULL; |
There was a problem hiding this comment.
Sanity, if wmode is NULL, wfilename should be freed before return.
| #include "utilmd5.h" | ||
|
|
||
| #ifdef _WIN32 /* --ak */ | ||
| extern FILE *ppu8open(const char *filename, const char *mode); |
There was a problem hiding this comment.
IMO would be perfect if such redefinitions were in some dedicated place, like utildecl.h, utilplat.h or a new one, containing only platform-dependent code.
| #include "utiliof.h" | ||
|
|
||
| #ifdef _WIN32 /* --ak */ | ||
| FILE *ppu8open(const char *filename, const char *mode); |
There was a problem hiding this comment.
Perhaps there should be util_fopen() function explicitly called in other places, instead of hiding fopen() by a macro?
src/util/utiliof.c
Outdated
| unsigned char *p; | ||
| size_t len = strlen(filename); | ||
|
|
||
| fnn = malloc(len + 10); |
There was a problem hiding this comment.
Or meybe unsigned char fnn[MAX_PATH + 10]?
There was a problem hiding this comment.
I have found a document about very long path name:
https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd
In short, very long file name is available after the \\?\ prefix and it could be longer than MAX_PATH.
So, util_malloc() should be better.
src/util/utiliof.c
Outdated
| return NULL; | ||
| } | ||
| if (wstr==NULL) { | ||
| wstr = malloc(sizeof(wchar_t)*(len+1)); |
There was a problem hiding this comment.
IMO we should consequently use util_malloc(), as probably all code in pplib. afair there was two reasons to avoid direct calls to malloc():
- easier replacement with some other base allocator (some day maybe)
- low-level check for NULL (and rather brutal solution..)
There was a problem hiding this comment.
I understand pplib defines util_malloc() at util/utilmem.c and is using util_malloc() instead of malloc().
I agree util_malloc() is better.
3 participants
I heard Akira Kakuto-san have decided to complete his contribution to the TeX community and have closed w32tex.org.
This pull request is the patch derived by @aminophen for Windows MS Visual C from Kakuto-san's W32TeX source code.
I expect the patch is also useful for other compilers mingw and cygwin on Windows.
I would like you to consider pulling it in the master.
The patch treats: