Alcaro / Flips

Floating IPS is a patcher for IPS and BPS files.
Other
334 stars 46 forks source link

Windows read only fix #14

Closed bbbradsmith closed 6 years ago

bbbradsmith commented 6 years ago

Was receiving a "read-only" error trying to apply a patch directly to a file instead of to another output file.

This change makes a copy of the inrom file data and closes it before proceeding to apply the patch.

The copy itself seems redundant, the problem is an unclosed file, but it was tied to the filemap/file class structure which seems to have to close the file and free its memory in one delete step, so this seemed like a smaller change than trying to adjust the many versions of those classes.

Also a few small syntax/compilation tweaks for MSVC.

A windows build of this is available here: (link removed)

Alcaro commented 6 years ago

The bloat exists on GCC on Windows (aka MinGW). I believe redefining new/delete is standard C++, but if MSVC does something silly with it, feel free to ifndef it on _MSC_VER.

Copying the data like that will increase the peak memory usage. I believe closing the file early is a better solution (i.e. commit dad6aff). If that doesn't work, fiddling with the file sharing flags would be better than an extra copy.

The other changes look good to me, though I'll probably regress those structs a few times... either it's part of the C++ standard, and MSVC whining is a bug, or it's a GCC extension, in which case GCC is the buggy one for not warning about extensions even with -std=c++98.

While I appreciate your intentions with those binaries, I'm not redistributing them unless I can verify that they're properly PGOed, dependency-free and virus-free (nothing personal, I'm paranoid against all binaries). Maybe I should just use Wine, neither GCC nor Flips does anything tricky enough that Wine bugs are a real concern... and releasing a new version is overdue anyways, the changelog since 1.31 is getting quite big...

bbbradsmith commented 6 years ago

Sure, I only provided the builds because your comment on https://github.com/Alcaro/Flips/issues/13 sounded like a request for it. If that wasn't the case and you'd prefer to build them yourself, then ignore them. I'll delete the links.

I can confirm the alternate fallback solution works for me, and agree it's better. I wasn't certain which file/filemap subclass applied, or whether it had to be amended in more than one place, or for other platforms besides the one I was building and testing.

As for the rest of it, I think the make.bat change should be taken. /nologo is not a valid argument for rc and causes it to fail, at least for the version of MSVC9 I have (which doesn't display the logo in question), and wouldn't change anything about the compilation result anyway.

The new and structs thing, I don't have much opinion about. Solve the way you see fit, or not at all if you want to ignore MSVC.

If you're interested, the error in the case of new/delete redefinition looks like this:

flips.cpp(11) : error C2373: 'operator new' : redefinition; different type modifiers predefined C++ types (compiler internal)(23) : see declaration of 'operator new'
flips.cpp(12) : error C2373: 'operator delete' : redefinition; different type modifiers predefined C++ types (compiler internal)(24) : see declaration of 'operator delete'

I didn't think it was worth spending time figuring out what this compiler expects that's different from what is there, since the given reason was a 200k larger binary which doesn't actually seem to happen with MSVC.

Same sort of deal with the structs. I don't really know whether or not that particular device is standard; I felt it was easier to do the trivial code manipulation than research aggregate syntax. The resulting error looks like this:

global.h(51) : error C2059: syntax error : '{'
global.h(51) : error C2143: syntax error : missing ';' before '{'
global.h(51) : error C2143: syntax error : missing ';' before '}'

Feel free to close.

Alcaro commented 6 years ago

make.bat is useless unless MSVC can compile the entire thing. I don't know what MSVC expects from a custom operator new, but it's easy to ifdef.

I suspect the struct thing is standard in C99, but not in C++, and GCC implemented it in both because why not. The GCC guys have always liked C way more than Microsoft.