Removing intel preprocessors from qhull_a.h

[frenchwr]

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;

[tacaswell]

attn @ianthomas23 @mdboom thoughts?

[WeatherGod]

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]

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]

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]

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.