utelle / wxsqlite3

wxSQLite3 - SQLite3 database wrapper for wxWidgets (including SQLite3 encryption extension)
http://utelle.github.io/wxsqlite3
Other
589 stars 178 forks source link

Fixes for MSVS warnings, and to allow vcpkg to know about DLL Debug and DLL Release #87

Closed ga2k closed 4 years ago

ga2k commented 4 years ago

@utelle I coded up a change or two here because vcpkg didn't know how to handle the Configurations that didn't start with Debug or Release (DLL Debug and DLL Release). It was a simple one line change in each PropertyGroup.

I also added a #pragma warning(disable : 4267) to stop VS from complaining about narrowing from size_t to int. I put it at the very top of sqlite3_amalgamation.c but move it to wherever you want it.

This is my first ever Pull Request, be gentle... :-)

utelle commented 4 years ago

I coded up a change or two here because vcpkg didn't know how to handle the Configurations that didn't start with Debug or Release (DLL Debug and DLL Release). It was a simple one line change in each PropertyGroup.

The problem here is that the VS build files are generated via premake5. Adjusting the build files manually is therefore not an option. That is, I have to find a way to persuade premake5 to generate the required vcpkg instructions.

3 configurations are provided to support the typical build configurations:

If vcpkg checks only for the configuration prefix, then an alternative solution could be to rename the configurations to Debug DLL / Release DLL. IMHO renaming the configurations should usually do not harm.

I'll have to check how vcpkg distinguishes between static and DLL builds, so that we don't run into new problems, when adding the component to vcpkg later on.

I also added a #pragma warning(disable : 4267) to stop VS from complaining about narrowing from size_t to int. I put it at the very top of sqlite3_amalgamation.c but move it to wherever you want it.

AFAIK these warnings are issued only in 64-bit builds. I'm not a fan of suppressing warnings. The better approach would certainly be to avoid those warnings altogether. I will look into this.

This is my first ever Pull Request, be gentle... :-)

Don't worry. I always appreciate if someone is taking the time to improve a component. Even if I don't apply a PR (or only after modifications), the accompanying discussions often lead to improvements.

ga2k commented 4 years ago

Yes, renaming the builds as Debug DLL and Release DLL will remove the need to make those changes to vcxproj. vcpkg looks for a subset of possible name prefixes to determine if it should link against. AFAIK they are Debug, Release and RelWith. As far as static and dynamic, I only use dynamic, so I can't tell you if static linking would work.

You're right about the warning only being in 64 bit builds, because I think 32 bit builds still use 32 bit size_t's. I didn't want to venture too far into *amalgamation.c, because that file is the based mostly on sqlite3.c and any changes to the "guts" would have to be make every time you recompile against a new version of it. I can see that coming to grief sooner or later.

Where is sqlite3 hosted? Perhaps a PR there would do it, but I'd leave that up to you. But there are only six or seven warnings, honestly I can just shut my eyes and pretend I never saw them...

Thanks for taking a look.

utelle commented 4 years ago

Yes, renaming the builds as Debug DLL and Release DLL will remove the need to make those changes to vcxproj.

Ok, I will consider to rename the configurations.

You're right about the warning only being in 64 bit builds, because I think 32 bit builds still use 32 bit size_t's. I didn't want to venture too far into *amalgamation.c, because that file is the based mostly on sqlite3.c and any changes to the "guts" would have to be make every time you recompile against a new version of it. I can see that coming to grief sooner or later.

The amalgamation is generated. So, any fixes should be applied to the original sources (which are now part of a separate project, SQLite3 Multiple Ciphers).

Where is sqlite3 hosted? Perhaps a PR there would do it, but I'd leave that up to you. But there are only six or seven warnings, honestly I can just shut my eyes and pretend I never saw them...

I will look into it. It shouldn't be too hard to make those warnings go away.

utelle commented 4 years ago

Commit 1afc8d545bf2acdad4625425049dbe51b7be032a should eliminate the compiler warnings in 64-bit builds. The changes are all related to the encryption extension which is not part of the official SQLite distribution, but of SQLite3 Multiple Ciphers.

I will decide later what to do about the configuration names and vcpkg. For the time being I hope you can live with your manually adjusted .vcxproj file(s).

ga2k commented 4 years ago

Thx for looking