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

[C++] Uriparser will not compile using Intel compiler #26474

Closed
asfimport opened this issue Nov 5, 2020 · 8 comments
Closed

[C++] Uriparser will not compile using Intel compiler #26474

asfimport opened this issue Nov 5, 2020 · 8 comments

Comments

@asfimport
Copy link

Fix for the Uriparser header file. The macro "URI_INLINE" was defined to "__force_inline", however the intel compiler keyword is now "__forceinline". Because of this change, the Intel compiler was no longer recognizing the keyword, which caused the whole build to fail. The patch is just changing this one line in the UriDefsConfig.h file from


# define URI_INLINE __force_inline

to


# define URI_INLINE __forceinline

The source for the new key word can be found here.
I have a fork ready to make a pull request.

Environment: SUSE Linux Enterprise Server 12 SP3 with Intel Compiler Stack
Reporter: Jensen Richardson / @jensenrichardson
Assignee: Jensen Richardson / @jensenrichardson

PRs and other links:

Note: This issue was originally created as ARROW-10503. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
You can definitely submit a PR. Does it have to check the compiler version? I would also suggest reporting the issue upstream.

@asfimport
Copy link
Author

Jensen Richardson / @jensenrichardson:
I already reported it to the Uriparser team. Unfortunately, I don't have the documentation for the compilers older than 19, but my stack is built on 18 (the version denotes the year for the Intel compilers) and it still worked. I think it is unlikely enough that people are using version older than 18, so I feel comfortable enough not checking, and I wouldn't even know where the cutoff for using the old syntax would be because they don't say online. Frankly, the Intel historical documentation is terrible, and I'm unable to find anything online for any version older than the newest.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
Can we just use inline instead of __forceinline on icc? Presumably that would work with all recent versions of the compiler.

@asfimport
Copy link
Author

@asfimport
Copy link
Author

Wes McKinney / @wesm:
I pretty commonly see FORCE_INLINE macros in codebases nowadays, it seems that inline alone will not suffice but would be good to have some definitive documentation about why that is

@asfimport
Copy link
Author

Jensen Richardson / @jensenrichardson:
I would defer to the Uriparser team on why they chose the force inline option. I'm just updating it for the new syntax.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
I don't know about upstream Uriparser, but URI parsing is not performance-critical for Arrow, so we should not worry about the performance of "force_inline" vs. "inline", IMHO.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
Issue resolved by pull request 8600
#8600

@asfimport asfimport added this to the 3.0.0 milestone Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant