Skip to content

Cleanups#78

Merged
ViralBShah merged 6 commits intoJuliaMath:masterfrom
NuxiNL:cleanups
Jan 10, 2015
Merged

Cleanups#78
ViralBShah merged 6 commits intoJuliaMath:masterfrom
NuxiNL:cleanups

Conversation

@EdSchouten
Copy link
Copy Markdown
Contributor

Hi there,

Attached is a small number of random, not so strongly related cleanup changes. A couple of these are actually needed for my use-case (fenv.h, OPENLIBM_ONLY_THREAD_SAFE). The others are just some cleanups for things I noticed along the way.

After these changes get merged, I no longer need to patch any OpenLibm source files. I only need tweak the Makefiles here and there.

Thanks,
Ed

I grepped through the FreeBSD source tree and for me, it seems to be
totally unclear why these two specific functions are weak references.
Such a construct is commonly used by FreeBSD's threading library
(libthr) to override certain functions, but I can't find any traces of
that.

Just use the function name directly. This fixes a compiler warning as
well (-Wmissing-prototypes).
…s().

- Add missing prototypes to openlibm.h for sincos() and sincosf().
- Mark the internal kernel functions static.
The global signgam variable is only part of the X/Open System
Interfaces. It is not part of the POSIX base definitions nor the C
standard.

I'd rather have it disabled for my specific use-case, so introduce a new
compilation flag that we can use to disable it.
OpenLibm uses the __weak_reference() macro for platforms where double
and long double use the same layout. That way functions only need to be
provided by the library once. The point is, in this specific case we
want to use strong references; not weak references.

Strong references can be used to give a symbol a second name. If you
look at the resulting object file, you will have two symbols with the
same offset and size. Weak references are different, in the sense that
they are marked in such a way that they act as fallbacks. They are only
used if an explicitly matching symbol is missing.
I guess the idea would be to eventually also install all of the
openlibm*.h headers, instead of just openlibm.h. Make openlibm_fenv.h
suitable for this purpose by moving all of the $ARCH/fenv.h headers next
to it.

We actually need this change to make OPENLIBM_USE_HOST_FENV_H work.
Right now it's still broken, because the "#include <fenv.h>" performed
by openlibm_fenv.h still pulls in $ARCH/fenv.h as $ARCH/ is added to the
compiler include path.
@EdSchouten
Copy link
Copy Markdown
Contributor Author

Oh, wait. It turns out that our __strong_reference() macro only works if the target symbol has the same type as the source. Let me remove that part of the change for now.

Edit: Done.

…eeded."

Unlike the __weak_reference() macro, __strong_reference() does type
checking. It can only create the reference if the type of the source and
the destination function match exactly.

Even if double == long double in practice, they remain unequal at the
language level.
ViralBShah added a commit that referenced this pull request Jan 10, 2015
@ViralBShah ViralBShah merged commit f418d26 into JuliaMath:master Jan 10, 2015
@ViralBShah
Copy link
Copy Markdown
Member

Do you think the header files are all better off living in include/? Also, if we end up using the new header files, do they need to be installed as part of the make install step?

@EdSchouten
Copy link
Copy Markdown
Contributor Author

Thanks for merging!

Yes. I think having the files in include/ would be a bit better. I would even consider moving the internal headers that are stored into include/ into src/ instead. We currently use them the wrong way around in my opinion. The reason why I decided to move the headers into src/ was only because openlibm.h was already stored there.

As you mentioned, we should also add the install targets.

Would you be interested in addressing these issues, or shall I send you a patch?

By the way, I now only have a very small number of local changes against OpenLibm left. I'm only removing a couple of files from SRCS for functions that don't need to be built in my environment. I don't think there is anything useful that can be merged into OpenLibm itself. Feel free to release a new version of OpenLibm. Thanks!

Ed

@ViralBShah
Copy link
Copy Markdown
Member

You are right to do it the other way around. I realised it as I was looking at it. It would be great if you can send a PR.

Thanks again for all the patches. These tremendously increase the quality of 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.

2 participants