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

CLN: explicitly cast (void *) -> (char *) #4524

Merged
merged 1 commit into from Jun 14, 2015

Conversation

tacaswell
Copy link
Member

When pulling data from from PyArrray_DATA explicitly cast
to (char *).

Addresses issues raised in #4518

@frenchwr does this address the issues of using icpc for everything? I was inspried by the `-fp-model strict' which I (maybe wrongly) translated to 'pointer model' given that the exception is about mis-matched pointer types.

When pulling data from from PyArrray_DATA explicitly cast
to (char *).

Addresses issues raised in matplotlib#4518
@tacaswell
Copy link
Member Author

@efiring @jkseppan blame says the two of you have worked on this bit of code before.

@jkseppan
Copy link
Member

Looks correct to me.

@efiring
Copy link
Member

efiring commented Jun 14, 2015

As far as I understand C, there is nothing "unclean" about the existing version here; but the explicit casts don't hurt anything. (In my understanding, the point of having a void pointer is that explicit casts are not needed. Otherwise, to be consistent, you would also need to use a cast with every assignment of NULL.)
This seems like the wrong solution to the original problem, however, which is that there is a chunk of C++ marooned in a C source file (in qhull). Running everything through a C++ compiler seems like an odd hack.

@jkseppan
Copy link
Member

I was wondering why the NULL doesn't need a cast, but I guess that expands to nullptr and is handled specially in C++.

@efiring
Copy link
Member

efiring commented Jun 14, 2015

Must be; but the basic point is that this is C code, not C++. And as C code, it doesn't need the explicit casts.

@jkseppan
Copy link
Member

I agree that the casts are unnecessary when compiling as C, but if this file is otherwise valid as C++ I don't see the harm in adding the casts. I also agree that the proper fix is to make qhull valid C.

@frenchwr
Copy link

Yep, this did the trick! Is there a way to force the use of icpc in the build config? Otherwise I think the default build will fail, which includes installs from package managers like pip.

CC=icpc python setup.py install

@efiring
Copy link
Member

efiring commented Jun 14, 2015

Given that this change does no harm, I will merge it. But I also merged #4519, so it shouldn't be necessary unless someone really wants to force the compiler to be icpc.

@tacaswell
Copy link
Member Author

@efiring Ah, learned something about C, I have mostly worked in C++.

@frenchwr The 'easy' cop out there is to install all the dependencies first.

So, it seems we have two ugly but functional solutions (this and #4519 ).

efiring added a commit that referenced this pull request Jun 14, 2015
CLN: explicitly cast (void *) -> (char *)
@efiring efiring merged commit 62bef78 into matplotlib:master Jun 14, 2015
@tacaswell tacaswell deleted the cln_fixup_cntr_pointers branch June 14, 2015 21:33
@QuLogic
Copy link
Member

QuLogic commented Jun 16, 2015

the `-fp-model strict' which I (maybe wrongly) translated to 'pointer model'

FYI, this refers to the floating point model (strict IEEE754, denormal handling, etc.)

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.

None yet

5 participants