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 <[email protected]> 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 > what? > 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: > > http://sourceforge.net/projects/mingwbuilds/files/host-windows/releases/4.8.1/64-bit/threads-win32/seh/x64-4.8.1-release-win32-seh-rev3.7z/download > 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
