On Wed, Sep 25, 2013 at 4:48 PM, G M <[email protected]> wrote: > On Thu, Sep 26, 2013 at 7:18 AM, Reid Kleckner <[email protected]> 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 <[email protected]> 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 >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
