[PATCH] [libcxx] Distinguish between MSVC, MSVCRT, and Windows-specific code
rnk at google.com
Mon Aug 19 11:49:36 PDT 2013
This patch has lots of unrelated stuff in it.
For example, the s/__restrict__/__restrict/ patch seems like obvious
goodness and is really easy to review on its own.
However, it's mixed in with bits like the "#if 0" change to math_win32.h
which I don't understand the need for.
I'd recommend resending a list of individual patches. For this kind of
portability work, I think it will make it easier for people testing
different configurations to identify and report any breakage.
On Thu, Aug 15, 2013 at 10:55 AM, G M <gmisocpp at gmail.com> wrote:
> Nico, you said, quoting your whole message:
> "You can't just intentionally break other platforms in the process. You
> should also mention that it's for Mingw. The attached patch restores the
> previous behavior. But it doesn't link, at least for me. How are you
> building libc++ with Mingw?"
> Your comments about breaking "other platforms" would sound better if you
> hadn't already broke "other platforms" a week ago. Which remain broken for
> all that I know.
> They'd also sound even better if they weren't ambiguous. About an
> uncommitted patch of mine trying to unbreak a committed patch of yours? Or
> They'd be better still if they weren't of alarmist language and had even
> a screen scrape error message of what "not linking" actually means.
> "not linking" says nothing. Undefined symbols? Multiply defined symbols?
> Symbols related to anything I actually changed in my patch?
> What platform is your focus? Testing on what platform? All things you like
> to ask but never divulge.
> So Nico, keeping it simple. I can't help you. Try being more helpful, less
> terse and alarmist next time. And if you were commenting on my patch, which
> is hard to tell from the terseness, try including an actual screen scrape
> of what problem you are actually seeing next time so I and anyone see can
> see if it sounds even related to my patch.
> Then someone has a chance of fixing it and helping you. Otherwise you just
> give the impression my patch has a problem when it might or might not
> still and that's the end of it. Not helpful.
> Depending on what version of mingw and clang and gnu etc. etc. you have,
> anything might not link and it might be totally fine depending on what you
> know to be already working or broken and what it is you are actually link
> against to form an .exe if at all. libcxx / clang on widnows is kina beta
> in my opinion. If someone wants to argue otherwise, I'll defer to them. But
> in the light I see it, it's beta. So being all alarmist and terse really
> just slows a slow process that has few interested people in already, down
> to a treacle; which might explain why things have been broken for a week
> and counting. It might not too.
> So Nico, back to where I was before your non contribution:
> Dear Win32/libcxx expert people, I found another small issue with my patch
> and fixed it. This patch aims to fix whatever Nico broke before and a few
> other random things besides.
> It's not without risk given my amateur hacker status and it just
> represents my own random whims and my attempts to fix build errors in tools
> that I find.
> Take each change on it's own merit and if someone wants to query each and
> every change that's fine by me.
> If you dear expert / or nico want to build libcxx from svn with mingw
> here's what I use today:
> But try building before my patch and after.
> Similarly do the same with VS2013 preview as I also used that.
> But please don't ask me how to set mingw up, I'm not an expert and it's a
> distraction. Buf if you know and have a build bot that's configured this,
> do tell, because then me and Nico can both go look at it.
> Here's my patch details but the code and diff file are the last word.
> * Small changes to __config to add what seems to be an omitted option that
> suits VS.
> * Added include to <algorithm> for MSVCRT since it seems to now use
> builtins that otherwise seem not be present.
> * lgamma cmath that seems to be needed to compile now. I forget why but
> maybe related to howard or marshal change.
> * cstdio snprintf is a macro on VS/MINGW. It causes a compile problem
> without this change.
> * cstdlib need win32 support.h not locale.h here to compile as a possible
> result of my rejigging to support nico's change.
> * limits_win32, as a result to marshall change I think. I needed to define
> __CHAR_BIT__ for VS only not clang.
> * win32 locale.h changed about __restrict I found beneficial in testing
> with VS2013 and g++ and clang++.
> * win32 support.h
> * also to remove strtof etc. because newer versions of C bases support so
> removing them prevents duplicate errors.
> * changed _Exit from a macro to an inline because it was causing compile
> errors. probably should remove support IMHO.
> * support.cpp removed excess throw stuff I added sime time ago causing
> warnings and was IMHO excessively defensive.
> * some rejigs to remove nuisance warnings.
> * some 80 column width reshaping.
> Thanks, mingers / Win32's. May our ranks ever decrease..
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits