[PATCH] D25145: [libc++] Correct explanation of _LIBCPP_NEW_DELETE_VIS

[Shoaib Meenai via cfe-commits]

smeenai closed this revision.
smeenai added a comment.

Not sure why Phabricator isn't closing this out, but this was committed as SVN 
r284016


https://reviews.llvm.org/D25145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25145: [libc++] Correct explanation of _LIBCPP_NEW_DELETE_VIS

[Shoaib Meenai via cfe-commits]

smeenai added a comment.

In https://reviews.llvm.org/D25145#565027, @mclow.lists wrote:

> Sigh. Make an expedient choice that you don't really agree with, and you get 
> immediately reminded of it.   I suggested on an earlier review (not this 
> patch) that I really didn't want to see `_WIN32` in any files other than 
> ``, that we should have a libc++-specific one.  I let that slide 
> because we had a couple other instances of `_WIN32` in the source base.
>
> About three days later, this pops up. :-(
>  Not your fault, but it makes me sad.


I'm not a fan either :) Gonna commit this and then send something to cfe-dev 
about cleaning them up.


https://reviews.llvm.org/D25145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25145: [libc++] Correct explanation of _LIBCPP_NEW_DELETE_VIS

[Marshall Clow via cfe-commits]

mclow.lists added a comment.

Sigh. Make an expedient choice that you don't really agree with, and you get 
immediately reminded of it.   I suggested on an earlier review (not this patch) 
that I really didn't want to see `_WIN32` in any files other than ``, 
that we should have a libc++-specific one.  I let that slide because we had a 
couple other instances of `_WIN32` in the source base.

About three days later, this pops up. :-(
Not your fault, but it makes me sad.


https://reviews.llvm.org/D25145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25145: [libc++] Correct explanation of _LIBCPP_NEW_DELETE_VIS

[Shoaib Meenai via cfe-commits]

smeenai added a comment.

My initial understanding of the problem was incorrect; I did some further 
experimentation and I think I properly understand what's going on here now. 
Sorry for the churn.


https://reviews.llvm.org/D25145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25145: [libc++] Correct explanation of _LIBCPP_NEW_DELETE_VIS

[Shoaib Meenai via cfe-commits]

smeenai created this revision.
smeenai added reviewers: compnerd, EricWF, mclow.lists.
smeenai added a subscriber: cfe-commits.

The behavior of this macro actually needs to apply universally on
Windows and not just when using the Microsoft CRT. Update the macro
definition and documentation accordingly.


https://reviews.llvm.org/D25145

Files:
  docs/DesignDocs/VisibilityMacros.rst
  include/new


Index: include/new
===
--- include/new
+++ include/new
@@ -125,7 +125,7 @@
 
 }  // std
 
-#if defined(_LIBCPP_MSVCRT) && !defined(_LIBCPP_BUILDING_LIBRARY)
+#if defined(_WIN32) && !defined(_LIBCPP_BUILDING_LIBRARY)
 # define _LIBCPP_NEW_DELETE_VIS
 #else
 # define _LIBCPP_NEW_DELETE_VIS _LIBCPP_FUNC_VIS
Index: docs/DesignDocs/VisibilityMacros.rst
===
--- docs/DesignDocs/VisibilityMacros.rst
+++ docs/DesignDocs/VisibilityMacros.rst
@@ -112,14 +112,15 @@
   Mark a symbol as being exported by the libc++ library. This macro must be
   applied to all `operator new` and `operator delete` overloads.
 
-  **Windows Behavior**: When using the Microsoft CRT, all the `operator new` 
and
-  `operator delete` overloads are defined statically in `msvcrt.lib`. Marking
-  them as `dllimport` in the libc++ `` header is therefore undesirable: if
-  we were to mark them as `dllimport` and then link against libc++, source 
files
-  which included `` would end up linking against libc++'s `operator new`
-  and `operator delete`, while source files which did not include `` would
-  end up linking against msvcrt's `operator new` and `operator delete`, which
-  would be a confusing and potentially error-prone inconsistency.
+  **Windows Behavior**: The `operator new` and `operator delete` overloads
+  should not be marked as `dllimport`; if they were, source files including the
+  `` header (either directly or transitively) would lose the ability to 
use
+  local overloads of `operator new` and `operator delete`. On Windows, this
+  macro therefore expands to `__declspec(dllexport)` when building the library
+  and has an empty definition otherwise. A related caveat is that libc++ must 
be
+  included on the link line before `msvcrt.lib`, otherwise Microsoft's
+  definitions of `operator new` and `operator delete` inside `msvcrt.lib` will
+  end up being used instead of libc++'s.
 
 Links
 =


Index: include/new
===
--- include/new
+++ include/new
@@ -125,7 +125,7 @@
 
 }  // std
 
-#if defined(_LIBCPP_MSVCRT) && !defined(_LIBCPP_BUILDING_LIBRARY)
+#if defined(_WIN32) && !defined(_LIBCPP_BUILDING_LIBRARY)
 # define _LIBCPP_NEW_DELETE_VIS
 #else
 # define _LIBCPP_NEW_DELETE_VIS _LIBCPP_FUNC_VIS
Index: docs/DesignDocs/VisibilityMacros.rst
===
--- docs/DesignDocs/VisibilityMacros.rst
+++ docs/DesignDocs/VisibilityMacros.rst
@@ -112,14 +112,15 @@
   Mark a symbol as being exported by the libc++ library. This macro must be
   applied to all `operator new` and `operator delete` overloads.
 
-  **Windows Behavior**: When using the Microsoft CRT, all the `operator new` and
-  `operator delete` overloads are defined statically in `msvcrt.lib`. Marking
-  them as `dllimport` in the libc++ `` header is therefore undesirable: if
-  we were to mark them as `dllimport` and then link against libc++, source files
-  which included `` would end up linking against libc++'s `operator new`
-  and `operator delete`, while source files which did not include `` would
-  end up linking against msvcrt's `operator new` and `operator delete`, which
-  would be a confusing and potentially error-prone inconsistency.
+  **Windows Behavior**: The `operator new` and `operator delete` overloads
+  should not be marked as `dllimport`; if they were, source files including the
+  `` header (either directly or transitively) would lose the ability to use
+  local overloads of `operator new` and `operator delete`. On Windows, this
+  macro therefore expands to `__declspec(dllexport)` when building the library
+  and has an empty definition otherwise. A related caveat is that libc++ must be
+  included on the link line before `msvcrt.lib`, otherwise Microsoft's
+  definitions of `operator new` and `operator delete` inside `msvcrt.lib` will
+  end up being used instead of libc++'s.
 
 Links
 =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits