Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sqlite3 #46

Closed
wants to merge 7 commits into from
Closed

Sqlite3 #46

wants to merge 7 commits into from

Conversation

JesseKPhillips
Copy link
Contributor

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).

@andralex
Copy link
Member

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

@braddr
Copy link
Member

braddr commented May 16, 2011

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

@JesseKPhillips
Copy link
Contributor Author

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

@JesseKPhillips
Copy link
Contributor Author

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.

@braddr
Copy link
Member

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.

@JesseKPhillips
Copy link
Contributor Author

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
Copy link

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.

@andralex
Copy link
Member

What's the status of this? Thanks.

@JesseKPhillips
Copy link
Contributor Author

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

@JesseKPhillips
Copy link
Contributor Author

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. #64

@andralex
Copy link
Member

andralex commented Jun 4, 2011

I'm a gonna close this.

@andralex andralex closed this Jun 4, 2011
NilsBossung pushed a commit to NilsBossung/phobos that referenced this pull request Jun 15, 2013
ChangeD - the D changelog generator script
redstar added a commit to redstar/phobos that referenced this pull request Feb 11, 2018
[ltsmaster] Add missing definition for Solaris.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants