Skip to content

Move public headers into include/ and private headers into src/.#80

Merged
ViralBShah merged 2 commits intoJuliaMath:masterfrom
NuxiNL:header-dirs
Jan 11, 2015
Merged

Move public headers into include/ and private headers into src/.#80
ViralBShah merged 2 commits intoJuliaMath:masterfrom
NuxiNL:header-dirs

Conversation

@EdSchouten
Copy link
Copy Markdown
Contributor

While there, also modify the install target. We should make sure to
install all openlibm*.h headers. There is still some work to be done:
openlibm_fenv_*.h still depends on some additional bits. I'd propose
that we eventually create an include/openlibm_cdefs.h that contains all
of the macros we need.

While there, also modify the install target. We should make sure to
install all openlibm*.h headers. There is still some work to be done:
openlibm_fenv_*.h still depends on some additional bits. I'd propose
that we eventually create an include/openlibm_cdefs.h that contains all
of the macros we need.
@Keno
Copy link
Copy Markdown
Contributor

Keno commented Jan 11, 2015

Thanks! @ViralBShah, can we give commit access to @EdSchouten, seems more than deserved! This might require adding a new team, since it's really separate from the regular Julia team.

It seems I accidentally copied these headers twice in
d078203. We should only have them in
include/.
@ViralBShah
Copy link
Copy Markdown
Member

I think I did that yesterday and also gave access to @talex5. Is that not the case?

@Keno
Copy link
Copy Markdown
Contributor

Keno commented Jan 11, 2015

Oh, didn't know, I checked at some point yesterday, but you may not have done it then.

ViralBShah added a commit that referenced this pull request Jan 11, 2015
Move public headers into include/ and private headers into src/.
@ViralBShah ViralBShah merged commit ee29769 into JuliaMath:master Jan 11, 2015
@ViralBShah
Copy link
Copy Markdown
Member

BTW, I notice that in the tests, we have to include openlibm.h, openlibm_fenv.h and openlibm_complex.h. In the corresponding C headers, one does have to include all these, but for openlibm, should openlibm.h automatically include all the other openlibm headers?

Comment thread Makefile
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use these headers in julia on windows, these should be put back

@EdSchouten
Copy link
Copy Markdown
Contributor Author

Viral,

Right now openlibm.h is close to identical to math.h, so it doesn't automatically pull in openlibm_fenv.h and openlibm_complex.h. Something that came to mind was that we could rename openlibm.h to openlibm_math.h for consistency and add a very simple openlibm.h that does nothing more than include openlibm_{math,complex,fenv}.h. How does that sound to you?

Tony,

That's interesting. These headers are used by the build phase, but do we consider them to be public API? Would you mind explaining in what way Julia on Windows uses these? Thanks!

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Jan 11, 2015

No, they're not exactly public API, so it is perhaps incorrect to install them. We have a handful of assembly source files in Julia that only get used by the Windows build, which use these headers for a few assembly-related macro definitions. We could make independent copies of them in the julia repository, or refer to these files via their uninstalled source locations since we have openlibm as a submodule (this is actually what we're doing now), but I don't really like either of these options and would rather be using installed copies of the headers than private source locations.

@EdSchouten
Copy link
Copy Markdown
Contributor Author

I see. I would personally consider copying these files into the Julia repository if we eventually want OpenLibm to become an independent library.

Shall I add these install directives back to the Makefile for now, but add a TODO item there that we should reconsider this in the future?

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Jan 11, 2015

I think this can remain a Julia TODO for now. We may eventually want to try adapting OpenLibm so it is possible to compile with MSVC, which would require some tweaking of these assembly macros (and translating all of the assembly to Intel syntax), at which point we can revisit this problem. As long as these files don't change relative location I think it should still work, though whenever we next bump the submodule we'll definitely want to test it thoroughly in a PR first.

@ViralBShah
Copy link
Copy Markdown
Member

I think so long as we clearly mark the things we do not want to support long term, @tkelman and I can work towards removing those dependencies from base Julia, and then from openlibm.

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.

4 participants