Skip to content

Some optimizations and bug fixes #182

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

Closed
wants to merge 3 commits into from

Conversation

KOLANICH
Copy link
Contributor

No description provided.

@SRombauts
Copy link
Owner

Hi @KOLANICH, I've noticed that you reported a lot of issues and proposed some improvements. Thank you for your time!

For your Information, I am no more actively developing this library, but I welcome any help you can provide for improving it!

Cheers!

@SRombauts
Copy link
Owner

Could you perhaps separate this into multiple more specific merge request?

I feel this would help me integrate the parts I am the more confident in!

Cheers!

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Feb 13, 2019

Could you perhaps separate this into multiple more specific merge request?

I can, but not all of the commits. Some of them depend on each other and If I separated them there will be merge conflicts.

I also wonder if we should drop Travis CI. It is dead, they use ultra-obsolete Docker images.

Is this yours? I propose to

  1. make that repo synced with this one
  2. drop Travis support and setup GitLab CI instead of Travis one. Do you know any good and AUTOMATICALLY UPDATED to the latest versions of everything Docker images for that?

@SRombauts
Copy link
Owner

Yes, it's also mine repo on Gitlab, and I have actually been using Gitlab CI at my previous job, but right now I don't have the time to switch.

If I recall correctly, Travis is in fact now providing more recent Ubuntu images, it might be a matter of proper config at the top of the Travis yaml config file

@KOLANICH
Copy link
Contributor Author

but right now I don't have the time to switch.

So if I created a PR with .gitlab-ci.yml, you would have accepted it, would you?

If I recall correctly, Travis is in fact now providing more recent Ubuntu images, it might be a matter of proper config at the top of the Travis yaml config file

Yes, they have introduced

dist: xenial

in 2018 after bionic has been released and xenial has become obsolete.

@SRombauts SRombauts self-assigned this Mar 4, 2019
@SRombauts
Copy link
Owner

Hi @KOLANICH, I somehow missed your last comment... sorry for not answering!

I am going to cherry pick some of your commits, starting by simple things.

Regarding your most disruptive changes, yes it would be nice to simply throw every old compilers and platform, but that's rarely how things works in most companies; Ubuntu 18.04 is more recent then what I used at work until recently and at home.
We do also often use Debian or other distros.

So we could instead for a new v3 branch for instance, with disruptive changes, or requesting a much more recent setup (CMake & C++ version)

This (as much as other considerations) that I am not wiling to just drop Github, but I indeed would gladly add a gitlab-ci Yaml file

Thanks again for your time!

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Mar 4, 2019

Regarding your most disruptive changes, yes it would be nice to simply throw every old compilers and platform

I don't even know which of them is the most disruptive. I have rewritten the most of the template code in the way it doesn't require any variadic templates, is it was initally. pure and const attrs are supported by GCC-like compilers for quite a while (unfortunately const attr in c++17 syntax ([[const]], and even [[gnu:const]], unlike [[pure]], but [[pure]] is unsuitable for our needs, we need const) is not supported at all), so using them is not expected to be disruptive. IMHO the most disruptive is requiring the latest CMake introducing FetchContent (IMHO even with it CMake does damn bad, we need something like conan.io, but without any large dependencies, like python).

(__TI_COMPILER_VERSION__ > 7003000 && defined(__TI_GNU_ATTRIBUTE_SUPPORT__))\
)\
)
#if !defined(pure) and __has_attribute(const)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, here is the error (though causing no harm in most of cases). Should be !defined(SQLITECPP_PURE_FUNC)

@KOLANICH KOLANICH force-pushed the fixes_by_KOLANICH branch from 963ef3b to c275cb4 Compare May 7, 2019 17:26
@maxbachmann
Copy link
Contributor

maxbachmann commented May 9, 2019

Is it intended to drop support for compilers lower than c++11? (I do exactly the same for binding using enable_if in my project, so it's no problem for me, but I do not know whether there are still people with older compilers) and while I think the use of sqlite3_bind_parameter_index is a cool addition, but it should completely replace the current functionality, since at least I definetly do have a use case for the initialisation using a variadic template

@KOLANICH KOLANICH force-pushed the fixes_by_KOLANICH branch from c275cb4 to 8b58fe9 Compare June 11, 2019 17:35
@SRombauts
Copy link
Owner

Yes @maxbachmann, my intention is to drop support for compiler older than C++11.
I could start a new version 3.x of SQLiteCpp and I would do keep a branch for maintenance as needed.

@maxbachmann
Copy link
Contributor

@SRombauts yes it probably makes sence to drop the support in a major release :)

@KOLANICH KOLANICH force-pushed the fixes_by_KOLANICH branch from 8b58fe9 to c4d7e2c Compare July 16, 2019 14:39
@SRombauts
Copy link
Owner

Closing this one since it has been mostly merged by pieces by now.
Thanks again!

@SRombauts SRombauts closed this Jan 13, 2020
@KOLANICH KOLANICH deleted the fixes_by_KOLANICH branch January 13, 2020 21:53
@KOLANICH KOLANICH restored the fixes_by_KOLANICH branch January 14, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants