Skip to content

Remove throw spec from omp.h for clang - #558

Merged
chyunsu3 merged 1 commit into
dmlc:masterfrom
larroy:master
Sep 8, 2019
Merged

chyunsu3 merged 1 commit into
dmlc:masterfrom
larroy:master

Conversation

@larroy

@larroy larroy commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

This fixes compilation with OMP using the Clang compiler.

@chyunsu3

chyunsu3 commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

I suspect the memory error is due to race condition

@larroy

larroy commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

@hcho3 in CI ?

@chyunsu3

chyunsu3 commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

@larroy Yes, the memory error you can see in Travis CI is intermittent

@larroy

larroy commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

#560

@larroy

larroy commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

#559

@larroy

larroy commented Sep 8, 2019

Copy link
Copy Markdown
Contributor Author

@hcho3 can we merge this? I need it for an MXNet PR.

@chyunsu3
chyunsu3 merged commit 7ce90a3 into dmlc:master Sep 8, 2019
@chyunsu3

chyunsu3 commented Sep 8, 2019

Copy link
Copy Markdown
Contributor

Merging this for now. I'll create a new issue about failing tests for dmlc_exception and std_exception.

@larroy

larroy commented Sep 8, 2019

Copy link
Copy Markdown
Contributor Author

seems it's not failing due to double free anymore.

@leezu

leezu commented Dec 7, 2019

Copy link
Copy Markdown
Contributor

@larroy should this be #if defined(__ANDROID__) || defined (__clang__) instead of #if defined(__ANDROID__) && defined (__clang__). The second doesn't make much sense to me and in fact doesn't work with clang

@larroy

larroy commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

I think the intention was to fix it for Android only, when using clang, hence &&, don't remember the details. But see that we are compiling with Clang in CI for a while, why did it start failing now?

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