[PATCH] [libcxx] Distinguish between MSVC, MSVCRT, and Windows-specific code

Howard Hinnant hhinnant at apple.com
Mon Aug 19 10:33:54 PDT 2013


On Aug 15, 2013, at 1:55 PM, 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 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..
>  
>  
> <libcxx4.diff>_______________________________________________

Sorry for the delay in reviewing this patch.  Looks fine to me.  I'm counting on all those interested in this platform to have reviewed this by now.

I have one tiny nit:  I'd prefer in <algorithm> to move:

#ifdef _LIBCPP_MSVCRT
#include "support\win32\support.h" // __builtin
#endif


up in the header just a few lines just under:

#if defined(__IBMCPP__)
#include "support/ibm/support.h"
#endif

and just above:

#include <__undef_min_max>

That is:

#if defined(__IBMCPP__)
#include "support/ibm/support.h"
#endif

#ifdef _LIBCPP_MSVCRT
#include "support\win32\support.h" // __builtin
#endif

#include <__undef_min_max>


Any problems with that?  Just trying to keep the includes together and I'd like to have __undef_min_max last.

Otherwise, I'm ready to commit this.  Thanks for working on it!

Howard




More information about the cfe-commits mailing list