Skip to content

various changes for removing warning in clang compilation: const char…#16

Merged
adamrankin merged 1 commit intoPlusToolkit:masterfrom
eruffaldi:master
Jul 27, 2018
Merged

various changes for removing warning in clang compilation: const char…#16
adamrankin merged 1 commit intoPlusToolkit:masterfrom
eruffaldi:master

Conversation

@eruffaldi
Copy link
Copy Markdown
Contributor

@eruffaldi eruffaldi commented Jul 19, 2018

Fix of building error under macOS clang 9.1.0 and warnings under Linux with GCC 4.9.2

Comment thread ndicapi.cxx
if (pol->ErrorCallback)
{
pol->ErrorCallback(errnum, ndiErrorString(errnum), pol->ErrorCallbackData);
pol->ErrorCallback(errnum, (char*)ndiErrorString(errnum), pol->ErrorCallbackData);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, the return type of ndiErrorString is char*, so this is redundant. Is it the ndicapiExport token in ndicapiExport char* ndiErrorString(int errnum); that causes the warning?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see, changes to ndiErrorString below, nevermind.

Comment thread ndicapimodule.cxx
#define PyString_Format PyUnicode_Format
#define PyString_AsString PyUnicode_AsUTF8
#define PyIntObject PyLongObject
//#define PY_INT_OBJECT_OB_IVAL(ob) PyLong_AsLong((PyObject*)(ob))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove commented code. If you want to keep it, please add a comment above as to why it's being kept.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The safe way to get the long inside a PyLong is to use PyLong_AsLong(PyObject*) and not just ob_digit[0]. All around in CPython PyLong_AsLong is used.
https://github.com/python/cpython/blob/6405feecda6a5d5dd7a4240eb3054a2676ed29b1/Objects/longobject.c

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you know more about this than I do. Whatever you think is right (and works on all platforms).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed with the most compatible way: PyLong_AsLong

Comment thread ndicapimodule.cxx
bitfield_lshift(PyIntObject* v, PyIntObject* w)
{
register unsigned long a, b;
register long a, b;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this a bug fix or a compiler warning? If compiler warning, please #ifdef it to OSX vs other

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Compiler mega-warning when doing (b < 0) that's impossible. These variables got assigned as from ob_digit[0] from PyLong that is a signed long and not unsigned.

I would also remove the register keyword.

Comment thread ndicapimodule.cxx
Py_NDIErrcodeMacro(NDI_EPROM_READ);
Py_NDIErrcodeMacro(NDI_EPROM_WRITE);
Py_NDIErrcodeMacro(NDI_EPROM_ERASE);
//Py_NDIErrcodeMacro(NDI_EPROM_READ);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do these no longer work? If so, could you add an issue?

If they do work, please undo the commenting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed due to missing symblols. I can protect them with an #ifdef

@adamrankin
Copy link
Copy Markdown
Member

Thank you very much for the fixes! I've added some comments and questions to the PR.

@adamrankin
Copy link
Copy Markdown
Member

After a couple of #ifdefs, let me know and I'll merge it.

…const char, error in probe python, || and &&

added BX to Python
@adamrankin adamrankin merged commit c37d61d into PlusToolkit:master Jul 27, 2018
@adamrankin
Copy link
Copy Markdown
Member

Thanks for the contribution, the python wrapping is not my area!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants