[patch] [libcxx] _LIBCPP_WEAK

[G M]

Hi Reid (question at the end for everyone)

Thanks for your time on this.

Ok, well I think it's progress to know it's a stylistic issue. And I agree
it's Howard's call on if to apply "default" to declarations AND definitions.

If we do have to reapply default on the definition I'll use the macro that
matches it's header.

But I'm still a little unsure if _LIBCPP_NEW_DELETE_VIS is what
_LIBCPP_FUNC_VIS maps to in all situations. If it does, should we stop
using the former and use the latter instead?

BTW, this would be a great clang tool to go around apply "default" to the
definitions where it's on the declarations?! Then those that like one style
or another can chop and change.

To be useful for our situation, it might need to be a little clever. It'd
have to do something like look at the prototype and the definition to see
which already has "default" applied either directly or through a macro.
Then offeri to apply that attribute or macro to the "other" one; and report
any issues where it sees a conflict or is dubious.

Any thoughts on that (anyone)? Does such a tool already exist?


On Thu, Sep 26, 2013 at 12:15 PM, Reid Kleckner <rnk at google.com> wrote:

> On Wed, Sep 25, 2013 at 4:48 PM, G M <gmisocpp at gmail.com> wrote:
>
>> On Thu, Sep 26, 2013 at 7:18 AM, Reid Kleckner <rnk at google.com> wrote:
>>
>>> I'm OK with this because it helps libc++ build, but I can't yet see a
>>> path forward for actually implementing replaceable operator new on Windows.
>>>
>>> I agree that you can drop the visibility("default") bits from new.cpp,
>>> because it's already in the header.  I would probably duplicate
>>> _LIBCPP_NEW_DELETE_VIS on the definitions in new.cpp for consistency and
>>> documentation, since the visibility of new and delete is one of the most
>>> interesting things about it.
>>>
>> Just to be clear here, you're agreeing I "can drop" the
>> visibility("default") (implied: in theory?) , but that I "shouldn't" for
>> documentation purposes (in practice.). Is that correct?
>>
>> Second, I'm ok to do that, _LIBCPP_NEW_DELETE_VIS expands "default" as
>> does _LIBCPP_FUNC_VIS.
>>
>> but we don't put "default" attributes on definitions anywhere else in
>> libcxx, it'd be quite irregular. So why here and not there?
>>
>> Unless _LIBCPP_NEW_DELETE_VIS expands to something other than
>> _LIBCPP_FUNC_VIS in some situations that I've missed?
>>
>> If _LIBCPP_NEW_DELETE_VIS always does expand to _LIBCPP_FUNC_VIS, do we
>> need the former?
>>
>> And if we're going to put _LIBCPP_X_VIS attributes on definitions here,
>> shouldn't we also be putting them on definitions everywhere else? But that
>> would be a big task.
>>
>> I kind of agree it does help documentation, but it seems we are either
>> inconsistent if we do as you suggest, and if we fix that inconsistency we
>> have a big job to do to do that everywhere.
>>
>> So with that all said? Could you double confirm what you think here?
>>
>
> I don't actually feel strongly about this.  It's up to Howard.
>
>
>>  The changes to __config to detect RTTI and implement _LIBCPP_WARNING
>>> are unrelated and shouldn't be in this patch.
>>>
>>
>> It's true, it's just I'm hoping the other patches will be applied prior
>> and this would make merging easier. It certainly made my life easier. The
>> warning patch, you've LGTM'd I think, I only have a problem here if the
>> rtti patch doesn't get LGTM'd. Is that correct?
>>
>
> I stamped it, FWIW.  It fell out of my inbox.
>
>
>> Which is a good time to ask, are you ok with the RTTI patch. It fixes
>> ninga to build "out of the box" amongst other things. Without it, the
>> libcpp was failing to recognize -rno-rtti option in some cases.
>>
>> Thanks
>>
>>
>>>
>>>
>>> On Wed, Sep 25, 2013 at 6:42 AM, G M <gmisocpp at gmail.com> wrote:
>>>
>>>> Hi Everyone
>>>>
>>>> The attached patch is for libcxx's new.cpp and __config files. The
>>>> patch's intent is to make new.cpp compile using MS's cl.exe compiler
>>>> without changing the meaning of anything for any other compiler.
>>>>
>>>> The issue this patch seeks to address is that MS's compiler (cl.exe)
>>>> doesn't support the __attribute__((__weak__)) or
>>>> __atribute__((__visibility__("default")) syntax; so a solution must be
>>>> found where cl.exe doesn't see this syntax.
>>>>
>>>> This patch seeks to solve this problem by changing code patterned like
>>>> this:
>>>> __attribute__((__weak__, __visibility__("default")))
>>>> void* operator new(size_t size, const std::nothrow_t&) _NOEXCEPT {
>>>> /*snip*/; return p; }
>>>>
>>>> to code like this:
>>>> _LIBCPP_WEAK
>>>> void* operator new(size_t size, const std::nothrow_t&) _NOEXCEPT {
>>>> return p; }
>>>>
>>>> with the expectation that this change will NOT introduce any
>>>> functionality change for clang++/g++ etc. That expectation is based on two
>>>> aspects of the change:
>>>> * The first is the belief that cl.exe doesn't support "weak" in any
>>>> documented way and that libcxx on Windows doesn't need it
>>>> anyway. So _LIBCPP_WEAK is defined as nothing when cl.exe is the detected
>>>> compiler.
>>>>
>>>> For all other compilers, _LIBCPP_WEAK is defined to be just
>>>> __attribute__((__weak__)) and nothing more).
>>>>
>>>> This should mean that cl.exe doesn't see the weak attribute syntax and
>>>> so won't choke on it; and g++/clang++ will see the same weak attribute that
>>>> it saw before this patch.
>>>> * The second part is what to do about
>>>> __attribute__((_visibility__("default"))) as in the proposed change it is
>>>> dropped from the function definition.
>>>>
>>>> The expecatation here is that this is ok because it isn't neccessary
>>>> because the prototype for the modified functions already have it; so the
>>>> right thing should still happen.
>>>> If all of this is correct, then this patch should fix new.cpp for
>>>> cl.exe without changing anything else.
>>>>
>>>> It also provides a pattern that will work with all the compilers libcxx
>>>> already supports; and without having to introduce alternate #if/#else
>>>> guards or other uglyness. This should make it better match the patterns
>>>> libcxx already uses.
>>>>
>>>> If removing the "default" attribute turns out to be a problem, I
>>>> believe the default attribute could be added back now that it is decoupled
>>>> from the "weak" attribute (which I think is a good thing in of itself) by
>>>> using one of libcxx's existing macro's such as _LIBCPP_FUNC_VIS /
>>>> _LIBCPP_NEW_DELETE_VIS etc.
>>>>
>>>> I'm not sure of the neccessity of LIBCPP_NEW_DELETE_VIS or it's
>>>> realtionship to _LIBCPP_FUNC_VIS at this point, FWIW, but that doesn't
>>>> matter to the logic of this patch.
>>>>
>>>> I compiled this patch with cl.exe, g++ and clang++.exe.
>>>> Please let me know what you think. If this patch doesn't get traction,
>>>> I'd appreciate some advice with real alternative code that could be used to
>>>> advance things here as I found it hard to produce something actionable from
>>>> the comments I received to my previous patch for this problem though I did
>>>> and do appreciate the responses.
>>>>
>>>> Thanks
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130926/1c79763f/attachment.html>