Skip to content

Small patch to allow compiling on systems without log2#348

Closed
sambler wants to merge 3 commits intoAcademySoftwareFoundation:masterfrom
sambler:master
Closed

Small patch to allow compiling on systems without log2#348
sambler wants to merge 3 commits intoAcademySoftwareFoundation:masterfrom
sambler:master

Conversation

@sambler
Copy link
Copy Markdown
Contributor

@sambler sambler commented Apr 27, 2012

When creating the freebsd port for oiio I found I needed to add log2 functions to two files for freebsd 8.2 and earlier. Rather than make it freebsd specific I opted to check for log2 being defined so it can be more universal.

@c42f
Copy link
Copy Markdown
Contributor

c42f commented Apr 27, 2012

I'm afraid this may not be portable. My copy of the linux man pages for the posix standard math.h say:

"The following shall be declared as functions and may also be defined as macros."

with a long list of function names, including log2. So I don't think we can rely on log2 being a macro.

The right solution is probably to have this inside an #ifdef FreeBSD or something.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 27, 2012

Agreed with Chris, and I'll also add:

log2f is defined in fmath.h (conditionally, for Windows only). I think that's where you should put it, but in a #ifdef FreeBSD block. And then you won't need any changes for imageviewer.cpp.

I'm leery of including fmath.h in the ptex code -- because that's a totally separate package we have incorporated, and we'd want to keep it in sync with its master and not introduce any dependencies on OIIO to it. So in that case, I think it's best to have the #ifdef FreeBSD with the definition of log2 and any other missing FreeBSD items.

I can do all this really easily if you'd like, it's just a couple of lines.

With those changes, the basic idea LGTM.

@sambler
Copy link
Copy Markdown
Contributor Author

sambler commented May 2, 2012

I would have thought within #if defined(log2) would be sufficient. The log2 functions are available on freebsd v8.3, v9.0 and HEAD so an #if defined(FreeBSD) && (FreeBSD < 803000) block would be needed.

Not sure when linux added the log2 functions but I would have thought linux versions released more than a year or two ago would have the same problem.

@c42f
Copy link
Copy Markdown
Contributor

c42f commented May 2, 2012

The problem is that the log2 macro is /never/ defined on linux, regardless of whether log2 exists as a function!

Not to mention that we have to support windows too and I've no idea what happens there. Yup, writing cross platform code can be a pain ;-)

@sambler
Copy link
Copy Markdown
Contributor Author

sambler commented May 2, 2012

ok I admit I didn't even think of log2 as being #defined - I was thinking of the function name being tested in the defined() test. It appears that redefining the log2 function after math.h doesn't cause an error (without specific flags?).

Checking existence of log2 should fall back to cmake? Then #if in a log2 function when needed.

Also I forgot to mention before - don't recall why I picked "fmath.h" to start with, but it can be changed to <math.h> - still needed to bring in log and log2 when available.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented May 3, 2012

Unfortunately, the defined() is a preprocessor directive that only checks if the named preprocessor macro is #defined. C/C++ don't have a compile-time introspection that would allow you to know if a true function was defined (that sure would be nice).

If it were very complicated, we could have CMake check for the existence of log2, but in this case -- it exists on all but a couple configurations, and we know which ones those are -- I think it's easier to just add the #if to test for those systems directly.

sambler added 2 commits May 8, 2012 16:13
this commit only tests for freebsd versions other - others to be added as needed
@sambler
Copy link
Copy Markdown
Contributor Author

sambler commented May 8, 2012

This should be a sufficient start to listing system versions that need log2 functions.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented May 20, 2012

LGTM. I'm squashing these to one commit, moving the definition from imageviewer.cpp to fmath.h, and merging into master and RB-1.0.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented May 20, 2012

Merged.

@lgritz lgritz closed this May 20, 2012
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