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

Alternative flags and encoding support #93

Merged
merged 8 commits into from
Feb 21, 2017

Conversation

zauguin
Copy link
Collaborator

@zauguin zauguin commented Feb 18, 2017

Provides the functionality of #72 with the sqlite_config interface.

Fixes #71

AUTO = SQLITE_ANY,
UTF8 = SQLITE_UTF8,
UTF16 = SQLITE_UTF16
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin Use the same name as sqlite.

enum class Encoding {
  ANY = SQLITE_ANY,
  UTF8 = SQLITE_UTF8,
  UTF16 = SQLITE_UTF16
};

struct sqlite_config {
int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin Use an enum for flags instead of int.

enum class Flags {
  OPEN_READONLY = SQLITE_OPEN_READONLY,
  OPEN_READWRITE = SQLITE_OPEN_READWRITE,
  OPEN_CREATE = SQLITE_OPEN_CREATE,
  OPEN_NOMUTEX = SQLITE_OPEN_NOMUTEX,
  OPEN_FULLMUTEX = SQLITE_OPEN_FULLMUTEX,
  OPEN_SHAREDCACHE = SQLITE_OPEN_SHAREDCACHE,
  OPEN_PRIVATECACH = SQLITE_OPEN_PRIVATECACH,
  OPEN_URI = SQLITE_OPEN_URI
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a enum class operator | isn't defined, so this leads to quite some boilerplate. Do need operator& too?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin I didn't know that, Is there a workaround for this limitation?
No we don't need operator &.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin I just tested a hackish way, let me know what you think

enum class test { A = 1, B = 2, C = 4 };

test operator|(const test& a, const test& b) {
  test ret;
  ret = static_cast<test>(static_cast<int>(a) | static_cast<int>(b));
  return ret;
};

int main() {
  test c = test::A | test::B;
  cout << static_cast<int>(c) << endl; // prints 3 as expected :-)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I integrated it.

struct sqlite_config {
int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
const char *gVfs = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin rename this to zVfs same as sqlite documentation.

}

database(std::string const & db_name): _db(nullptr) {
database(const std::u16string &db_name, const sqlite_config &config = {}): _db(nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the default encoding for this constructor UTF16, this changes the behavior we had in previous versions.
@zauguin

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this comment. By default the encoding is ANY, so in line 434 we select encoding UTF16.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin You are right, my mistake :-)

@aminroosta
Copy link
Collaborator

@zauguin Looks very good to me, please add a unit test and update documentation.
Thanks for the great work you have done with these recent PRs 😉

@zauguin
Copy link
Collaborator Author

zauguin commented Feb 19, 2017

This will hit the same Visual C++ bug as #94.
To implement the work-around I would refactor the conversion into a separate method. Then we could make it a little bit more generic. Is there interest in wstring and u32string support?

@aminroosta
Copy link
Collaborator

@zauguin I thinks its better to keep it simple, std::string & std::u16string are enough :-)

@zauguin
Copy link
Collaborator Author

zauguin commented Feb 20, 2017

I only add tests for read-only, I don't know anything about the other ones, but there is no reason why they shouldn't work.

@zauguin zauguin changed the title [WIP] Alternative flags and encoding support [RDY] Alternative flags and encoding support Feb 20, 2017
@zauguin zauguin changed the title [RDY] Alternative flags and encoding support Alternative flags and encoding support Feb 20, 2017
OPEN_SHAREDCACHE = SQLITE_OPEN_SHAREDCACHE,
OPEN_PRIVATECACH = SQLITE_OPEN_PRIVATECACHE,
OPEN_URI = SQLITE_OPEN_URI
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aminroosta What do you think about omitting OPEN_? It looks a bit redundant. If you prefer to keep it we might move it to the class name: OpenFlags::READONLY looks better than Flags::OPEN_READONLY

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zauguin OpenFlags::READONLY looks better, I'm for it 😉
BTW do you think we should make a new release after this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I'm working on std::variant support and I would really like to see this in the next release, especially for supporting functions with different argument types, but apart from that, go for it.

@zauguin zauguin merged commit 51edf04 into SqliteModernCpp:master Feb 21, 2017
@zauguin zauguin deleted the flags branch February 21, 2017 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants