[cfe-dev] Fwd: [libc++] Should we guard against macro definitions

[Ben Craig via cfe-dev]

Short story:

Instead of putting lots of parenthesis everywhere, I think the better approach is to put lots of #ifdef + #undef blocks everywhere.  More rationale follows.

 

 

That's not good enough, because we have to "hoist" the function into namespace std.

 

Yes and no.

 

First the good(?) part.  This C implementation starts with a real function definition for ‘cos’, then follows it up with the macro version.  That means that “using ::cos;” does get the “double cos(double)” signature into namespace std, because it doesn’t invoke the macro (no parenthesis).

 

The bad part is that the cos macro is still hanging around, and if someone types something reasonable like “float x = cos(1.0f);” they will invoke the double version.  If they type “float x = std::cos(1.0f);” they will get a gross compiler error.

 

This suggests to me that the better answer is to throw around a lot of blocks of the form #ifdef cos, #undef cos.

 

libc++'s <cmath> header does this for some of the calls there (look at 'fpclassify', for example).

 

Doing the same for cos (and others) shouldn't be too hard.

 

I think C99 fpclassify is specified to be a macro, and C++ fpclassify is specified to be a set of functions.  Given that, I think the #undef technique used there is reasonable.  I don’t think we can do the same for the math functions though, because if we #undef the old cos, then introduce a new cos overload that takes a double as the only argument, then we will introduce ambiguity.  If we just do the #undef though, then I think we’ll be fine, as long as the cos macro isn’t the only way of getting to the C libraries cos implementation.

 

Can you provide a list of the functions that your library defines as macros?

 

I’m not sure if I can, but either way, it’s a rather spotty list.  For example, sinhf has a macro version, but sinf doesn’t.  I think it may make more sense to look at everything in the C version of tgmath, and guard all of those.  I’m absolutely willing to be the one to make the patch, I was just trying to gauge interest before going through the motions.

 

 

Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

 

From: cfe-dev [mailto:cfe-dev-bounces at lists.llvm.org] On Behalf Of Marshall Clow via cfe-dev
Sent: Wednesday, September 30, 2015 2:01 PM
To: cfe-dev at lists.llvm.org
Subject: [cfe-dev] Fwd: [libc++] Should we guard against macro definitions

 

forgot to cc the list on my reply.

 

-- Marshall

 

---------- Forwarded message ----------
From: Marshall Clow <mclow.lists at gmail.com <mailto:mclow.lists at gmail.com> >
Date: Wed, Sep 30, 2015 at 11:58 AM
Subject: Re: [cfe-dev] [libc++] Should we guard against macro definitions
To: Ben Craig <ben.craig at codeaurora.org <mailto:ben.craig at codeaurora.org> >

 

 

On Tue, Sep 22, 2015 at 9:44 AM, Ben Craig via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> > wrote:

I have a C library, and I want to build libc++ and libc++abi against it.  This C library’s implementation of many of the math.h functions involves code similar to…

#define cos(Val) RealCosImpl(Val)

 

This interacts poorly with libcxx’s cmath header, as the float and long double overload declarations hit this #define.

 

If I make a patch that guards the bulk of cmath from this kind of macro, do people feel that this would be useful, or would it get rejected on the grounds that it is unnecessary noise to support an odd C library?  This fix would generally take the form of putting the function name in parenthesis to suppress function macro expansion.  So code that looks like this… 

float cos(float val)

… would turn into this…

float (cos)(float val)

 

That's not good enough, because we have to "hoist" the function into namespace std.

 

 

One could argue that this would be a reasonable thing to do across libcxx, but I think it is more important to do so for cmath, as many of those functions must have macro version in the C tgmath.h header (but not the C++ one).  It probably wouldn’t hurt to throw some parenthesis at std::min and std::max in <algorithm> as well, considering the unfortunate history with those and the <windows.h> min and max macros.

 

libc++'s <cmath> header does this for some of the calls there (look at 'fpclassify', for example).

 

Doing the same for cos (and others) shouldn't be too hard.

Can you provide a list of the functions that your library defines as macros?

 

-- Marshall

 

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150930/64c27016/attachment.html>