search

lvandeve / lodepng

PNG encoder and decoder in C and C++.
zlib License
2.06k stars 421 forks source link

Small tweaks #57

Closed zeptoZB closed 6 years ago

zeptoZB commented 6 years ago

Removed warnings. Small tweaks for better 64-bit compatibility. Small tweaks for build in Visual Studio.

stinos commented 6 years ago

Better split this into commits, 1 for each logical change. Btw there are already multiple pull requests for the warnings/64-bit compatibility issues. You should probably also explain why you need to tweak the buid for VS: I doubt this would get merged without any explanation of why the changes are needed.

zeptoZB commented 6 years ago

The changes are minimal, in my opinion very simple to understand, looking at the changes is easier to understand than a long text explaining the reason for each change ... But that's how I work... There is no right or wrong, what is good for one may be unsatisfactory for another...

stinos commented 6 years ago

The changes are indeed very simple to understand, as in: sure it's easy to see what has changed. The why might be way less obvious though, especially for other people. Also because you're burrying behaviour changes like the change in line 1174 in a quite big diff, in a commit named 'small tweaks'. That's quite a long way from just 'unsatisfactory' as far as commit organization/messages go.

zeptoZB commented 6 years ago

I closed the request because it has alternate paths (disabling some warnings in Visial Studio) and will only keep the relevant changes ...