Remove unused output connection string from SQLDriverConnect call

[jon-v]

Passing the output connection string pointer to SQLDriverConnect was causing crashes when connecting asynchronously. Since this variable was entirely unused inside nanodbc, the quickest and best fix seemed to be to simply remove it.

[mloskot]

Although this change looks good to me for now, we may require the returned connection string in future features. So, it would be good to understand why it was causing crashes when connecting asynchronously. Any ideas?

[mloskot]

NULL should read nullptr

[jon-v]

NULL should read nullptr

There's quite a few instances of NULL instead of nullptr across the codebase (probably most of them from me, sorry about that). Do you mind if I leave this as-is for now and correct them all in a separate PR?

[mloskot]

Sure, thanks.

[jon-v]

Although this change looks good to me for now, we may require the returned connection string in future features. So, it would be good to understand why it was causing crashes when connecting asynchronously. Any ideas?

It's quite straightforward - the pointer being passed is to a local variable which goes out of scope immediately following the SQLDriverConnect call. When connecting asynchronously the function will return immediately, but the ODBC driver will try to write to the address later, probably when SQLCompleteAsync is called.

If we wanted to save the output connection string it would be as easy as keeping the buffer as a member variable in the connection_impl object instead.

[mloskot]

@jon-v

(...) SQLDriverConnect call. When connecting asynchronously the function will return immediately (...)

I see. I don't have experience with async connections and I thought this particular call might actually be blocking.

[mloskot]

One more thing, MSDN on SQLDriverConnect mentions OutConnectionString parameter can be NULL:

If OutConnectionString is NULL, StringLength2Ptr will still return the total number of characters (excluding the null-termination character for character data) available to return in the buffer pointed to by OutConnectionString.

But, it does not indicate NULL is allowed for StringLength2Ptr.
Is that an overlook in MSDN and can we be sure all drivers will accept NULL?

[jon-v]

Is that an overlook in MSDN and can we be sure all drivers will accept NULL?

History has shown that we can't even be sure that Microsoft's own drivers match their documentation. All I know is it works on our end, and the CI tests still pass.

[mloskot]

Yes, that's fair comment.

Let's wait for @lexicalunit vote.

[lexicalunit]

Yeah this is a good catch here. I've never been able to test with async myself so I never would have caught this problem. Thanks for the PR @jon-v! LGTM, we can either address the NULL vs nullptr issue in this PR or a subsequent one.

[mloskot]

Great, thanks @jon-v !

I wonder if we could add a CI build focused on testing the async stuff. Any guidelines would be helpful.