Skip to content

Removing intel preprocessors from qhull_a.h #4519

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 14, 2015
Merged

Removing intel preprocessors from qhull_a.h #4519

merged 1 commit into from
Jun 14, 2015

Conversation

frenchwr
Copy link

I'm removing this section of code that caused builds with the Intel compiler to fail with the following error message:

icc -pthread -fno-strict-aliasing -O3 -fPIC -fp-model strict -fomit-frame-pointer -DNDEBUG -g -O3 -Wall -fPIC -DMPL_DEVNULL=/dev/null -DPY_ARRAY_UNIQUE_SYMBOL=MPL_matplotlib__qhull_ARRAY_API -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -I/usr/local/python2/2.7.8/x86_64/intel14/nonet/lib/python2.7/site-packages/numpy/core/include -I/usr/local/include -I/usr/include -I. -Iextern -I/usr/local/python2/2.7.8/x86_64/intel14/nonet/include/python2.7 -c src/qhull_wrap.c -o build/temp.linux-x86_64-2.7/src/qhull_wrap.o
In file included from src/qhull_wrap.c(10):
extern/qhull/qhull_a.h(106): warning #77: this declaration has no storage class or type specifier 
template <typename T> 
^

In file included from src/qhull_wrap.c(10):
extern/qhull/qhull_a.h(106): error: expected a ";"
template <typename T>
         ^

In file included from src/qhull_wrap.c(10):
extern/qhull/qhull_a.h(115): warning #12: parsing restarts here after previous syntax error
  void    qh_qhull(void);
                        ^

compilation aborted for src/qhull_wrap.c (code 2) 
error: command 'icc' failed with exit status 2

The issue is that Intel's C compiler (icc) cannot handle templates; however, forcing the use of Intel's C++ compiler (icpc) creates the opposite problem as it fails to interpret certain C syntax. See more details on the issue I opened here: #4518 (comment)

Removal of this code block appears fairly innocuous however I'm not clear on the author's intent in including it. It may be that there are some unforeseen consequences. Incorporating these changes into my local build did not introduce any new unit test errors or failures.

QHULL_UNUSED gets called a few times (in extern/qhull/io.c) and it appears to be reading in a custom boolean type in both cases:

[frenchwr@vmps12 matplotlib]$ grep QHULL_UNUSED -r *
extern/qhull/io.c:  QHULL_UNUSED(unbounded);
extern/qhull/io.c:  QHULL_UNUSED(unbounded);
extern/qhull/qhull_a.h:  >--------------------------------</a><a name="QHULL_UNUSED">-</a>
extern/qhull/qhull_a.h:#define QHULL_UNUSED(x) (void)x;

@frenchwr frenchwr mentioned this pull request Jun 12, 2015
Closed
@tacaswell
Copy link
Member

attn @ianthomas23 @mdboom thoughts?

@tacaswell tacaswell added this to the next point release milestone Jun 13, 2015
@WeatherGod
Copy link
Member

Takinga look at this a bit, it certainly doesn't make any sense that there should be any templates in this .h file, as it gets included by C source files.

I should point out that the qhull code is lifted entirely from the qhull project, and so a patch should probably get submitted there as well: https://gitorious.org/qhull. That said, there are a couple of PRs there that are languishing for 3 years...

@ianthomas23
Copy link
Member

I am not in favour of this change for two reasons.

Firstly, the code in the extern directory is taken from third-party projects and is not intended to be altered by us. When a new version of a third-party project is released, it should be possible for an mpl developer will minimal domain knowledge to copy the changed files to the extern directory without worrying about the content of those changes. If we change a file locally, such a third-party update implies an increased maintenance load as the mpl developer must understand some of the internal workings of the third-party project.

Secondly, a user who has a copy of the qhull source code installed on his/her system will be using that system copy when building mpl rather than the local copy that we ship with mpl. Hence a change to our local copy does not benefit all intel compiler users.

To address both of the above concerns, any change to mpl should be in either setupext.py or src/qhull_wrap.c. But I do not use the intel compiler so I don't have any suggestions on what such a change could be.

@frenchwr
Copy link
Author

Those are fair points. However, I'm not sure any change to setupext.py or src/qhull_wrap.c would address the fact that icc cannot compile C++ templates.

I agree with @WeatherGod that the best solution is to propose this change upstream to the qhull folks, but I suspect that this will fall on deaf ears. My feeling is that this block of code was hastily written without any testing with Intel compilers. That being said, I freely admit that I have not taken the time to fully explore/appreciate what this code block was intended for.

@efiring
Copy link
Member

efiring commented Jun 14, 2015

It looks like qhull is unmaintained. The change proposed here is a minor cleanup that affects only intel compilation. If someone starts maintaining qhull, and releases a new version without fixing this silly bug, and we bring in that new version without noticing the reintroduction of the bug, the worst that will happen is that someone like @frenchwr will run into the bug again. At that point, it can be fixed upstream--when there is an upstream.

efiring added a commit that referenced this pull request Jun 14, 2015
@efiring
Merge pull request #4519 from frenchwr/master
ddf4a88 
Removing intel preprocessors from qhull_a.h
@efiring efiring merged commit ddf4a88 into matplotlib:master Jun 14, 2015
@efiring efiring mentioned this pull request Jun 14, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@frenchwr @tacaswell @WeatherGod @ianthomas23 @efiring