thanks for your contribution. In addition to the comments by Daniel and
Peter, my comments are in line:
On 19.03.2014 22:37, Bob Kast wrote:
> In Windows, a socket is of type SOCKET, not int.
Please post this as a seperate patch as it is not WinCNG-specific.
> Redundant #define inline
It might be there for a reason. Please do not forget that there are
other Windows-compilers except Visual Studio. For example, I use MinGW
and MinGW-w64 to compile libssh2. It might be worth to add an #ifndef
around the #define in that case.
> You shouldn’t need Openssl to compile if you aren’t selecting it.
That makes perfect sense with Peter's recent changes to the backend
selection. We just need to make sure that LIBSSH2_OPENSSL is defined as
the default within all buildfiles in order to be backwards compatible.
> Putting the #pragma for the libraries works for both DLL and LIB versions.
This is rather compiler-specific to Visual Studio than
platform-specific. The #ifdef WIN32 ist not sufficient in that case,
since MinGW gcc compilers won't understand the pragma statements. I
would rather suggest adding the libraries to the AdditionalDependencies
element of the Link section within your Visual Studio project files.
> _libssh2_wincng_hash_update: the parameter needs to be const to match.
> It is interesting that C just gives a warning for that.
But is casting from (const unsigned char *) to (unsigned char *)
actually a real fix for this or just hiding things?
> pPaddingInfo value is undefined. Doc says it must be NULL if not used.
Good catch, that's correct.
> STATUS_SUCCESS unfortunately not defined if using non-driver includes.
I will have to take a look at this. It works for MinGW. Please give me
some time over the weekend to setup my own Visual Studio 2012 to build
libssh2 and verifiy your suggestions.
> _libssh2_wincng_hash_ctx and _libssh2_wincng_key_ctx: I found this
> confusing. Kind of a circular #define. I think this is more of what
> was intended.
This change is okay with me. I think I used defines, because it the
approach was used within other places of libssh2. Daniel, Peter, what do
> Forward declarations: without these the compiler complains for each call.
MinGW gcc does not complain and the other backends do not have them.
Again, please allow me some time to verify this.
> Pragma to suppress “possible loss of data” warnings.
A compiler-specific flag would could also be put into the Visual Studio
> New Files:
> -Libssh2.sln – solution file
> -Libssh2.vcxproj, libssh2.vcxproj.filters – project files for libssh2
> -Tests.vcxproj, tests.vcxproj.filters – project files for tests – I
> haven’t tested this.
> These are specifically for VS2013 and make it easy to create DLL/LIB,
> Debug/Release, 32/64 bit builds.
I think we should rather wait for Alexander Lamaison's CMake files since
they will allow us to create generic VS project files that are not tied
to a specific VS and SDK version. For example, I still use VS 2012 for
my own projects.
Received on 2014-03-20