Closed kdschlosser closed 5 years ago
OK I solved this issue. I replaced OpenZWave::ltrim and OpenZWave::rtrim in Utils.cpp with the following code
std::string& OpenZWave::ltrim(std::string &s)
{
size_t first = s.find_first_not_of(' ');
if (string::npos == first)
{
return s;
}
s = s.substr(first, s.length() - 1);
return s;
}
std::string& OpenZWave::rtrim(std::string &s)
{
size_t last = s.find_last_not_of(' ');
if (string::npos == last)
{
return s;
}
s = s.substr(0, last + 1);
return s;
}
I do not know quite yet if it works. It does compile without error. I have another issue I have to fix before I will be able to know if it works.
everything compiles without error. just need to check the various platforms and other versions of c++ to see if this is a good solution or not. if there is anyone willing to help out with that aspect of it. it would be a big help.
Okay, good find.
Imho the solution below keeps the original code but uses static_cast, seems pretty standard to me:
std::string& OpenZWave::ltrim(std::string& s) {
s.erase(s.begin(), std::find_if(s.begin(), s.end(),
static_cast<int (*)(int)>(std::isgraph)));
return s;
}
std::string& OpenZWave::rtrim(std::string& s) {
s.erase(std::find_if(s.rbegin(), s.rend(),
static_cast<int (*)(int)>(std::isgraph)).base(), s.end());
return s;
}
Tested on VS2019 in c++14, c++17 and "latest mode". Tested on a Mac in c++11 mode (enforced in the Makefile) and c++2a mode.
If it works for you, I can prepare a PR.
Unfortunately it does not compile without error.
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(95): error C2440: 'static_cast': cannot convert from 'bool (__cdecl *)(_Elem,const std::locale &)' to 'int (__cdecl*)(int)'
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(95): note: None of the functions with this name in scope match the target type
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(94): error C2672: 'std::find_if': no matching overloaded function found
C:\Users\Administrator\Documents\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(95): error C2780: '_InIt std::find_if(_InIt,const _InIt,_Pr)': expects 3 arguments - 2 provided
C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\include\algorithm(167): note: see declaration of 'std::find_if'
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(101): error C2440: 'static_cast': cannot convert from 'bool (__cdecl *)(_Elem,const std::locale &)' to 'int (__cdecl *)(int)'
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(101): note: None of the functions with this name in scope match the target type
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(100): error C2672: 'std::find_if': no matching overloaded function found
C:\GitHub\python-openzwave\openzwave\cpp\src\Utils.cpp(101): error C2780: '_InIt std::find_if(_InIt,const _InIt,_Pr)': expects 3 arguments - 2 provided
C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\include\algorithm(167): note: see declaration of 'std::find_if'
I was trying to do something similar and I could not get it to compile. I am not proficient in c++ so this was a tad over my head.
I also wanted to point out this as a possible other issue. I am not sure and please correct me if I am wrong. The original code as well as the changes above would be considered a lambda yes? and if so at what compiler version did c++ add support for lambdas? I just want to make sure there isn't a problem in older versions of the compiler. I believe there is an issue with compiling using the older versions and this may be one of the causes.
I did investigate using the __cplusplus preprocessor definition in order to implement the 2 different code blocks. But unfortunately there is no standard it. and it can return an incorrect version number. So without a means of knowing the compiler version I am not sure how to go about adding code based on the compiler version.
Thanks for testing... The issue with "isgraph" is that is (a) overloaded (b) one of the overloads is templated. This makes it difficult for the compiler to figure out which one to use. You say you are running VS 2017? Your compiler says it selected bool (__cdecl *)(_Elem,const std::locale &)' which is
This is the version with "locale": https://en.cppreference.com/w/cpp/locale/isgraph
And this is the one OZW wants to use: https://en.cppreference.com/w/cpp/string/byte/isgraph
Since I do not have that issue on my mac and also not on Windows 10 I wonder if you changed anything else to make VS 2017 happy...
Anyway, here is an idea, I pushed a test version as branch OpenZWave__trim with that change (*), on my fork of ozw so if you find the time maybe try to compile this:
git clone https://github.com/petergebruers/open-zwave.git -- branch OpenZWave__trim
Then build the solution "MinOZW" in cpp\examples\windows\MinOZW\vs2010 - that is how I test on Windows... When opening with VS2019 it asks to convert the project to latest toolset and that is what I do, then build and run the MinOZW project.
Lambda expressions are in C++11 but this is not a lambda, it is "just a cast". Lambda: https://en.cppreference.com/w/cpp/language/lambda
(*) Edit: for testing purposes I also add some spaces in NotificationCCTypes.cpp to see if it really trims as expected.
I am not running VS2017 technically speaking. I am running the build tools. (no IDE) So I am not compiling this by using the solution file.
It's complicated But I am handling some of the code updates for python-openzwave. and how the library gets compiled is from inside of python.
do you know if the solution has added any compiler arguments? maybe I am missing one. At the moment I do not have enough space in my drive to install VS (been working on migrating to a new drive) So I am not able to test that way.
I am also using Windows 7 and not windows 10. that shouldn't make any difference i would imagine.
This is the environment setup when the build takes place
Machine architecture: x64
Build architecture: x86
== Windows SDK ================================================
version: 10.0.17763.0
sdk version: 10.0.17763.0\
path: C:\Program Files (x86)\Windows Kits\10
-- Universal CRT -------------------------------------------
path: C:\Program Files (x86)\Windows Kits\10\
-- ATLMFC --------------------------------------------------
path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\ATLMFC
include path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\ATLMFC\include
lib path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023\ATLMFC\lib\x86
== Extension SDK ==============================================
path: C:\Program Files (x86)\Microsoft SDKs\Windows Kits\10\ExtensionSDKs
== TypeScript =================================================
path: None
== HTML Help ==================================================
path: C:\Program Files (x86)\HTML Help Workshop
== .NET =======================================================
version: v4.0.30319
-- x86 -----------------------------------------------------
version: v4.0.30319
path: C:\Windows\Microsoft.NET\Framework
-- x64 -----------------------------------------------------
version: v4.0.30319
path: C:\Windows\Microsoft.NET\Framework64
-- NETFX ---------------------------------------------------
path: C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\
x86 exe path: C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\
x64 exe path: C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\x64
== Visual C ===================================================
version: 14.0
path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC
-- Tools ---------------------------------------------------
version: 14.16.27023
path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Tools\MSVC\14.16.27023
redist path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Redist\MSVC\14.16.27012
-- F# ------------------------------------------------------
path: None
-- DLL -----------------------------------------------------
version: v140-14.16.27012.6
path: C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\VC\Redist\MSVC\14.16.27012\x86\Microsoft.VC141.CRT
== MSBuild ====================================================
version: 14.0
path: C:\Program Files (x86)\MSBuild\14.0\bin
== Python =====================================================
version: 2.7.15.final.0
architecture: x86
library: Python27.lib
libs: ['c:\\stackless27\\libs']
includes: [
'c:\\stackless27\\include',
'c:\\stackless27\\include\\Stackless',
'c:\\stackless27\\include\\Stackless\\core',
'c:\\stackless27\\include\\Stackless\\module',
'c:\\stackless27\\include\\Stackless\\pickling',
'c:\\stackless27\\include\\Stackless\\platf'
]
and here are the compiler arguments that are used.
"libraries": [
"setupapi",
"python27"
],
"extra_link_args": [
"/IGNORE:4098",
"/MACHINE:X86",
"/NOLOGO",
"/SUBSYSTEM:WINDOWS"
],
"extra_compile_args": [
"/Gy",
"/O2",
"/Gd",
"/Oy",
"/Oi",
"/Fd\"C:\\Users\\Administrator\\Documents\\GitHub\\python-openzwave\\openzwave\\builb\\lib_build\\OpenZWave.pdb\"",
"/fp:precise",
"/Zc:wchar_t",
"/Zc:forScope",
"/EHsc",
"/wd4251",
"/wd4244",
"/wd4101",
"/wd4267",
"/wd4996",
"/wd4351",
"/FS",
"/Zc:inline"
],
"define_macros": [
["WIN32", 1],
["_MBCS", 1],
["_LIB", 1],
["USE_HID", 1],
["_MT", 1],
["_DLL", 1],
["OPENZWAVE_MAKEDLL", 1],
["NDEBUG", 1]
],
"library_dirs": [
"c:\\stackless27\\libs"
],
"include_dirs": [
"C:\\Users\\Administrator\\Documents\\GitHub\\python-openzwave\\openzwave\\cpp\\src",
"C:\\Users\\Administrator\\Documents\\GitHub\\python-openzwave\\openzwave\\cpp\\tinyxml",
"C:\\Users\\Administrator\\Documents\\GitHub\\python-openzwave\\openzwave\\cpp\\hidapi\\hidapi"
]
if you would be willing to post whatever it is that gets passed to the compiler i can compare the 2 and see if I am missing anything.
in your fork are there any other changes you have made? or is it just to the Utils.py (other then adding the whitespace)
I got it.
the std::isgraph is an overload of isgraph in ctype.h.
#include <ctype.h>
std::string& OpenZWave::ltrim(std::string& s) {
s.erase(s.begin(), std::find_if(s.begin(), s.end(),
static_cast<int (*)(int)>(isgraph)));
return s;
}
std::string& OpenZWave::rtrim(std::string& s) {
s.erase(std::find_if(s.rbegin(), s.rend(),
static_cast<int (*)(int)>(isgraph)).base(), s.end());
return s;
}
I am going to try this code now to see if it compiles properly.
BINGO!!
That did it.
I understand what you are saying, I have been working on python-openzwave myself... I am beginning to understand how cython works :) I am also terribly low on diskspace on my windows system :(
I cannot help you with msbuild, sorry about that. I have used it maybe 10 years ago. I think you are on the right track with that include file.
Oh, you've got it. Super!
BTW can't you compile the unmodified ozw master branch, when you add /std:c++11 to the compiler flags of your build system (that would be somewhere in python-openzwave)? Im just asking, I really don't know if it would be easier or better to have that in the python-openzwave project, or get your or my C++17 patch added to open-zwave. Or maybe both. I have not discussed it yet.
well the simple fact is that ptr_fun has been removed in c++17 so it needs to be modified anyway.
I am not sure as to why the isgraph is doing what it is doing. There is no mention of an std::isgraph( int c ) there is only mention of the template overload of std::isgraph (charT c, const locale& loc) None that I could locate any way. if in fact there is not std::isgraph( int c ) then the use of std::isgraph may not be correct.
And the issue could pop up as well from someone else. so using specifically using the the isgraph from ctype is not incorrect. I do not know enough about c++ and it's internal workings to be able to make that determination.
i do not make use of any /std switches. maybe this is something I should do. I will have to mess about with it and see. I wrote the code for python-openwzve that handles the compilation of openzwave under Windows. It is designed to make an identical replica of the build environment that is created by whatever software they use to build the library. It supports the following software from 2008 to present - Visual C, Visual Studio, Visual C Build Tools or Visual Studio Build Tools. So the program does know what the version of the compiler they are using is. and that can be used to determine what /std switch needs to be placed if any.
I get your point about C++17, I'll prepare a PR and ask @Fishwaldo, if the "cast" fix I propose fully fixes your build problem. Do you need any other modification?
In linux/mac/bsd makefile language standard is forced, that is why I've mentioned it:
RELEASE_CPPFLAGS := -std=c++11
DEBUG_CPPFLAGS := -std=c++11
On my mac, VSC figures out it the definition of isgraph is in:
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_ctype.h
I haven't tried to debug the preprocessor so I don't know how it is included. I might do that. Let me sleep on it...
well i did stumble across this
https://en.cppreference.com/w/cpp/string/byte/isgraph
in there is has a code example
int count_graphs(const std::string& s)
{
return std::count_if(s.begin(), s.end(),
// static_cast<int(*)(int)>(std::isgraph) // wrong
// [](int c){ return std::isgraph(c); } // wrong
// [](char c){ return std::isgraph(c); } // wrong
[](unsigned char c){ return std::isgraph(c); } // correct
);
}
the std::isgraph still fails using that code example. but it does state that using the static cast is incorrect also
this does work
#include <ctype.h>
std::string& OpenZWave::ltrim(std::string& s) {
s.erase(s.begin(), std::find_if(s.begin(), s.end(),
[](unsigned char c){ return isgraph(c); }));
return s;
}
std::string& OpenZWave::rtrim(std::string& s) {
s.erase(std::find_if(s.rbegin(), s.rend(),
[](unsigned char c){ return isgraph(c); }).base(), s.end());
return s;
}
hehehe, right, so it is cast to unsigned but the signature of the function is signed. And actually all casts are deprecated - they say "use lambda". All this simply to remove spaces from strings 😄 Thanks for pointing that out... I'll give it a try on Mac and VS2019... This still requires you to include ctype.h?
yes it does
All this simply to remove spaces from strings
one of the reasons i like higher level languages.
Yes, Lua, Python, C#, Go... Build with lambda is OK on Mac and Windows, C++11 to 17, but I am still not sure why you have to add ctype.h explicitly while I don't have to do that. VS2019 navigates to C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\ctype.h (without modifying anything else except those 2 lines containing isgraph). I'd like to find out why.
trust me I would like to know the exact same thing. I have been staring at this for a while and I cannot figure out why i have to do an explicit include of ctype.h
have you tried to build python-openzwave at all from a windows box?
you have to use the dev 0.5.x branch.
and run python setup.py build --flavor=dev this will do a download of openwave 1.6. the compilations is going to fail with a boat load of errors but at least we will be able to see what happens when it compiles Utils.h
I will push to the branch in my fork in a bit. this way we can find out if it is environmental or if it is the compiler portion of python-openzwave. If you are willing of course,
I have never tested pyozw on windows, only on Mac/Linux. But I can try...
When I run a compile of Utils.cpp with VS2019 and add /showIncludes to the command line option, then filter the result:
Utils.cpp
Note: including file: C:\Users\peter\Documents\open-zwave\cpp\src\Defs.h
Note: including file: C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\string
Note: including file: C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\cctype
Note: including file: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\ctype.h
Interesting: Defs.h (ozw) -> string (visual studio) -> cctype (visual studio) -> ctype.h (Windows Kits)
For sure, the declaration we need is there:
_Check_return_ _ACRTIMP int __cdecl isgraph(_In_ int _C);
The full build options (set on project level in VS, the vcxproj files in open-zwave\cpp\build\windows\vs2010) to obtain that result are:
/GS /analyze- /W3 /Zc:wchar_t /I"..\..\..\src" /I"..\..\..\tinyxml" /I"..\..\..\hidapi\hidapi" /Zi /Gm- /Od /Fd"Debug\OpenZWave.pdb" /Zc:inline /fp:precise /D "WIN32" /D "_DEBUG" /D "_LIB" /D "USE_HID=1" /D "_MBCS" /errorReport:prompt /WX- /Zc:forScope /RTC1 /Gd /Oy- /showIncludes /MDd /std:c++latest /FC /Fa"Debug\" /EHsc /nologo /Fo"Debug\" /Fp"Debug\OpenZWave.pch" /diagnostics:classic
If you want even more detail, you use /P option to produce "\open-zwave\cpp\build\windows\vs2010\Debug\Utils.i" = 2.2 Mb and has all the lovely details.
I'll try to test your fork, but maybe with this information you can already start to play the game of "Spot the difference"?
Fixed in ae9b5a418d4bbe58e72299598e15c5f41361e778
👍
@Fishwaldo
Thanks for the fix. much appreciated
one question tho. there is no need to include ctype.h?
@petergebruers
I would check the compiler args you have used. But I think those are from building openzwave in debugging.
I am working on updating the code for python_openzwave to be complaint with openzwave 1.6. I have come across an issue. In the last release we had no issues compiling openzwave using VS 2010 and newer. this also includes Visual C as well as the Visual Studio Build tools.
I am currently running Visual Studio Build Tools 2017 and I have not had an issue in the past. Now with 1.6 I get these errors
upon first investigation I come to find that ptr_fun has been removed in c++17. I am not sure if this is the cause of the errors. I thought it would be worth mentioning. If anyone can shed some light on what could possibly be causing the errors it would be most helpful.
Thanks