Sqlite3 #46

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

JesseKPhillips commented May 15, 2011

This will be my first pull request here and could definitely use some review. But first I will say SQLite 3 does not compile on Win32 with dmc, see bug 89: http://bugzilla.digitalmars.com/issues/show_bug.cgi?id=89

This pull request includes the entire source code for SQLite 3.7.6.2 and bindings for D. It does not include a wrapper, just the C declarations.

The concerns I have with this request are the dmc bug above, whether it builds on MacOS and FreeBSD, and in general, did I correctly modify the makefiles. I also wondering if bindings should just be .di files, they really aren't need to be compiled into Phobos since they're just header files really (Oplink complained about multiple definitions).

Owner

andralex commented May 16, 2011

This is great work, thanks! One question - how are the licensing issues taken care of?

Owner

braddr commented May 16, 2011

This should follow the curl model, depend on the library, not import the entire source.

Contributor

JesseKPhillips commented May 16, 2011

SQLite is released under the public domain. http://www.sqlite.org/copyright.html

Contributor

JesseKPhillips commented May 16, 2011

Sorry, I clicked the bigger button.

Brad, what makes you say that, zlib's source is included in Phobos. I think this makes SQLite's availability to a much greater audience, since it will always be there with bindings to the version supported.

Owner

braddr commented May 16, 2011

Zlib's direct inclusion is ancient and predates anyone bothering to ask about it.

Libraries are exactly the right tool for managing separate modules of code. If you argue that sqlite should be included to make it handier to use, where's the line? Why not include dozens and dozens of libraries for exactly the same reason. Why do libraries even exist, just build all code into every app that needs them statically. It just doesn't scale.

It also doesn't scale in another dimension, maintainability. Every time another library is merged into phobos, we take on the burden of following the upstream library for updates and take on the responsibility of doing so.

No thanks.

Contributor

JesseKPhillips commented May 16, 2011

Then I'm really glad I include SQLite in this request since past discussions didn't really get into how it should be.

There has been an understanding that including bindings for some common libraries is a good idea. The maintainability for these is exactly the same. You'll have to follow the upstream library for updates. There is the potential of including dozens of these.

I didn't get an answer as to whether this was the way forward, so I made my own choice for these reasons. So if we are going to include bindings to make libraries easier to access, why not include the library when the license allows for it?

michelf commented May 19, 2011

A quick review makes me wonder about two small things. First:

string SQLITE_VERSION           = "3.7.6.2";
const int SQLITE_VERSION_NUMBER = 3007006;

Those two are #defines in the original C header, why aren't they enums in D? The next one is an enum however:

enum
string SQLITE_SOURCE_ID         = "2011-04-17 17:25:17 154ddbc17120be2915eb03edc52af1225eb7cb5e";

Why this one and not the two others?

Second:

struct sqlite3{};
struct sqlite3_mutex {};
struct sqlite3_stmt {};
struct sqlite3_value {};
struct sqlite3_context {};
struct sqlite3_blob {};
struct sqlite3_pcache {};
struct sqlite3_backup {};

Shouldn't those be opaque types (like struct sqlite3;) instead, like in the original C header file? As they are now, the compiler will let you create variables of those types on the stack or on the heap using new, which obviously shouldn't be done.

Owner

andralex commented May 28, 2011

What's the status of this? Thanks.

Contributor

JesseKPhillips commented May 28, 2011

Sorry, last weekend I took care of some other things, I should have a new request tonight or tomorrow morning.

Contributor

JesseKPhillips commented May 29, 2011

I created a new branch and pull request with the recommend changes, once that is pulled in I will close this request and be removing this branch. D-Programming-Language#64

Owner

andralex commented Jun 4, 2011

I'm a gonna close this.

andralex closed this Jun 4, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment