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

Add connection_parameters and ODBC_OPTION_DRIVER_COMPLETE option. #98

Closed
wants to merge 6 commits into from

Conversation

vadz
Copy link
Member

@vadz vadz commented Mar 8, 2013

These commits add connection_parameters class which is a simple container for the backend, connection string and possibly some options, i.e. simply name/value pairs. It changes the factory classes to accept connection_parameters instead of a simple string for future extensibility and updates the ODBC backend to honour the value of ODBC_OPTION_DRIVER_COMPLETE option to allow specifying the "driver completion" parameter of SQLDriverConnect().

vadz added 6 commits March 8, 2013 17:02
Right now this is just a trivial wrapper for the backend and connection string
and no extra functionality but it will be extended with connection options
support in the subsequent commits.

No changes in behaviour, this is a pure refactoring.

Signed-off-by: Vadim Zeitlin <vz-soci@zeitlins.org>
No real changes, but just use "class" for consistency.

Signed-off-by: Vadim Zeitlin <vz-soci@zeitlins.org>
…string.

There are still no real changes as only the connect string part of the
connection parameters is currently used, but this will make it possible to use
other information stored in connection_parameters from the backend code in the
future.

Also replace session::lastFactory_ and lastConnectString_ with a single
lastConnectParameters_ making the code slightly shorter and simpler.
It should be to connections.html, not the (non existent) basics.html.
Document the new session ctor taking connection_parameters and the class
itself in the references page.
Allow passing arbitrary options via connection_parameters and add support for
an option allowing to specify the "driver completion" mode, i.e. the degree of
interactivity of the driver, in the ODBC backend.

By default now do ask the user if the DSN doesn't include the user name and/or
password for the connection but the old behaviour is still available if
ODBC_OPTION_DRIVER_COMPLETE option is set to SQL_DRIVER_NOPROMPT before
opening the session.
@ghost ghost assigned vadz Mar 8, 2013
@mloskot
Copy link
Contributor

mloskot commented Mar 8, 2013

I checked your submission on Windows 8 building SOCI with VS2012 and tested ODBC (MySQL, PostgreSQL, SQL Server). The DB2 builds fine and the three test cases run fine as well, so DB2 is not affected.
Travis shows all green, so I assume it is fine on Linux too.

I assigned you to this pull request, so if no objections are raised, feel free to merge it.

I'm looking at the code now.

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

@vadz These are the only comments I have. I did have a few thoughts where we could improve it, but that would touch the more advanced solutions we have discussed on the list. There is no point doing it now, no time. So, it all looks very good to me.

Let's wait for votes from others.

: once(this), prepare(this), query_transformation_(NULL), logStream_(NULL),
lastConnectParameters_(parameters),
uppercaseColumnNames_(false),
isFromPool_(false), pool_(NULL), backEnd_(NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looked at what unkle clang says in the build 215.2:

/home/travis/build/SOCI/soci/src/core/session.cpp:46:27: warning: field 'pool_'
  will be initialized after field 'backEnd_' [-Wreorder]
  isFromPool_(false), pool_(NULL), backEnd_(NULL)
                               ^

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this, thanks for noticing. I wonder why didn't g++ warn about it, normally it gives these warnings too. Unfortunately I have no idea how to specify the compilation flags with cmake...

BTW, it would be nice to get rid of many other warnings currently given by g++...

Copy link
Contributor

Choose a reason for hiding this comment

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

You may take a look at src/cmake/SociConfig.cmake, feel free to update the flags.
I thoroughly agree, we should aim clean compilation, so let's not forget it #100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants