[PATCH] D50205: [libc++] Add availability markup for aligned new/delete

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50205#1188454, @EricWF wrote:

> How does this work when a user provides their own definitions? Does the 
> attribute from the declaration still produce a warning? If so, then I think 
> an in-compiler approach is better.


Yes. I do agree that not warning when the user provides their own definition is 
a better user experience, however I think that is already the behavior we have 
for sized new/delete (with `_LIBCPP_AVAILABILITY_SIZED_NEW_DELETE`). Is it any 
different?

If we were to go for an in-compiler approach, what would be the behavior we 
want? Any TU that uses the operator but defines it would not get a warning, but 
any TU that uses it without defining it would still get a warning, right? If 
so, it doesn't seem like such a huge improvement.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50205



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

[PATCH] D50276: [clang] Fix broken include_next in float.h

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339016: [clang] Fix broken include_next in float.h (authored 
by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50276?vs=159109&id=159299#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50276

Files:
  cfe/trunk/lib/Headers/float.h


Index: cfe/trunk/lib/Headers/float.h
===
--- cfe/trunk/lib/Headers/float.h
+++ cfe/trunk/lib/Headers/float.h
@@ -21,8 +21,8 @@
  *===---===
  */
 
-#ifndef __FLOAT_H
-#define __FLOAT_H
+#ifndef __CLANG_FLOAT_H
+#define __CLANG_FLOAT_H
 
 /* If we're on MinGW, fall back to the system's float.h, which might have
  * additional definitions provided for Windows.
@@ -157,4 +157,4 @@
 #  define FLT16_TRUE_MIN__FLT16_TRUE_MIN__
 #endif /* __STDC_WANT_IEC_60559_TYPES_EXT__ */
 
-#endif /* __FLOAT_H */
+#endif /* __CLANG_FLOAT_H */


Index: cfe/trunk/lib/Headers/float.h
===
--- cfe/trunk/lib/Headers/float.h
+++ cfe/trunk/lib/Headers/float.h
@@ -21,8 +21,8 @@
  *===---===
  */
 
-#ifndef __FLOAT_H
-#define __FLOAT_H
+#ifndef __CLANG_FLOAT_H
+#define __CLANG_FLOAT_H
 
 /* If we're on MinGW, fall back to the system's float.h, which might have
  * additional definitions provided for Windows.
@@ -157,4 +157,4 @@
 #  define FLT16_TRUE_MIN__FLT16_TRUE_MIN__
 #endif /* __STDC_WANT_IEC_60559_TYPES_EXT__ */
 
-#endif /* __FLOAT_H */
+#endif /* __CLANG_FLOAT_H */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50205: [libc++] Add availability markup for aligned new/delete

[Louis Dionne via Phabricator via cfe-commits]

ldionne abandoned this revision.
ldionne added a comment.

Nevermind, it looks like this patch is not necessary anymore since 
https://reviews.llvm.org/D45015 landed.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50205



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

[PATCH] D37302: [Headers] Define *_HAS_SUBNORM for FLT, DBL, LDBL

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D37302#1189576, @pirama wrote:

> Sorry this fell of my radar.  I've rebased the patch.
>
> Since this has been inactive for a while, lets wait for a couple of days to 
> see if there are any other comments.  If there are no objections, I'll submit 
> this on Wednesday.


Ok. FWIW, I was about to submit exactly the same patch when I found yours. This 
solves a problem for me.


Repository:
  rC Clang

https://reviews.llvm.org/D37302



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

[PATCH] D50341: [libcxx] Mark aligned allocation tests as XFAIL on old OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: vsapsai.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

Since r338934, Clang emits an error when aligned allocation functions are
used in conjunction with a system libc++ dylib that does not support those
functions. This causes some tests to fail when testing against older libc++
dylibs. This commit marks those tests as XFAIL.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341

Files:
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp


Index: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
===
--- 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
+++ 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
@@ -14,6 +14,12 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
 
 #include 
 
Index: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
===
--- 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
+++ 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
@@ -14,6 +14,12 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
 
 #include 
 
Index: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
===
--- 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
+++ 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
@@ -14,6 +14,12 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
 
 #include 
 
Index: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
===
--- 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
+++ 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
@@ -14,6 +14,12 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
 
 #include 
 


Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
@@ -14,6 +14,12 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL:

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: vsapsai.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

The current code enables aligned allocation functions when compiling in C++17
and later. This is a problem because aligned allocation functions might not
be supported on the target platform, which leads to an error at link time.

Since r338934, Clang knows not to define __cpp_aligned_new when it's not
available on the target platform -- this commit takes advantage of that to
only use aligned allocation functions when they are available.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344

Files:
  libcxx/include/__config
  libcxx/include/new
  libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp


Index: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+#include 
+
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#   error "libc++ should have aligned allocation in C++17 and up when 
targeting a platform that supports it"
+#endif
+
+int main() { }
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+!defined(__cpp_aligned_new)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__)
 #define _LIBCPP_HAS_DEFAULTRUNELOCALE


Index: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+#include 
+
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#   error "libc++ should have aligned allocation in C++17 and up when targeting a platform that supports it"
+#endif
+
+int main() { }
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDI

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

@phosek I don't understand how you can expect code compiled with new headers to 
link against an old dylib, unless you're setting the target platform, in which 
case anything that would fail to link will instead be a compiler error. I mean, 
we're adding stuff to the dylib from one version to the next, so _of course_ it 
won't always work.

Regarding the specific error you're encountering (`Undefined symbols for 
architecture x86_64: "operator delete(void*, std::align_val_t)", referenced 
from: [...]`), we're aware of it and this will in fact be fixed by 
https://reviews.llvm.org/D50344.


Repository:
  rC Clang

https://reviews.llvm.org/D45639



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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I must say I therefore don't understand the purpose of this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D45639



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

[PATCH] D47400: [libcxx] [test] Allow a standard library that implements LWG 1203 in istream.rvalue/rvalue.pass.cpp

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D47400



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

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 159903.
ldionne marked an inline comment as done.
ldionne added a comment.

Address vsapsai's comments


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344

Files:
  libcxx/include/__config
  libcxx/include/new
  libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp


Index: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+#include 
+
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#   error "libc++ should have aligned allocation in C++17 and up when 
targeting a platform that supports it"
+#endif
+
+int main() { }
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+(!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__)
 #define _LIBCPP_HAS_DEFAULTRUNELOCALE


Index: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+#include 
+
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#   error "libc++ should have aligned allocation in C++17 and up when targeting a platform that supports it"
+#endif
+
+int main() { }
Index: libcxx/include/new
===
--- libcxx/include/new
+++ libcxx/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+(!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__)
 #define _LIBCPP_HAS_DEFAULTRUNELOCALE
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/include/new:111-116
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606
 #define _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE
 #endif

vsapsai wrote:
> Maybe move this to `__config` too? This way we'll have 
> `__cpp_aligned_new`-related macros together.
The big difference is that `_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE` 
is only used in ``, whereas `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` was used 
in other files as well. Hence, I feel like it makes more sense to lift 
`_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` into `<__config>`, but not 
`_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE `.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344



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

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Louis Dionne via Phabricator via cfe-commits]

ldionne marked an inline comment as done.
ldionne added inline comments.



Comment at: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp:11
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11

vsapsai wrote:
> Initially `with_system_cxx_lib` made me suspicious because macro 
> `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` doesn't need specific dylib. And `XFAIL: 
> availability=macosx10.12` seems to be working.
> 
> [Documentation](https://github.com/llvm-mirror/libcxx/blob/master/docs/DesignDocs/AvailabilityMarkup.rst#testing)
>  describes which feature should be used in different cases but in this case I 
> cannot definitely say if test uses unavailable feature. I think it is 
> acceptable to stick with `with_system_cxx_lib` but I decided to brought to 
> your attention the alternative.
My understanding is that the `availability` feature may not be there if we're 
not using availability macros, since they can be disabled entirely.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344



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

[PATCH] D50341: [libcxx] Mark aligned allocation tests as XFAIL on old OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp:15
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8

vsapsai wrote:
> In what cases are we supposed to run these tests? Such extensive collection 
> of unsupported C++ standards looks suspicious and I guess that is the reason 
> why I haven't seen test failures with older libc++ dylibs.
That's an excellent question. I would assume those should be enabled in C++17 
and above, I'm not sure why they're disabled. @mclow.lists was the one to 
introduce those tests, perhaps he can shed light on why they were disabled in 
C++17?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added reviewers: mclow.lists, timshen.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

This commit fixes a regression introduced in r316095, where we don't match
inverted character classes when there's no negated characrers in the []'s.

rdar://problem/43060054


Repository:
  rCXX libc++

https://reviews.llvm.org/D50534

Files:
  libcxx/include/regex
  libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp


Index: 
libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+}
Index: libcxx/include/regex
===
--- libcxx/include/regex
+++ libcxx/include/regex
@@ -2427,7 +2427,6 @@
   const bool __in_neg_mask = (__neg_mask_ == 0) ||
   __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))


Index: libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+}
Index: libcxx/include/regex
===
--- libcxx/include/regex
+++ libcxx/include/regex
@@ -2427,7 +2427,6 @@
   const bool __in_neg_mask = (__neg_mask_ == 0) ||
   __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

The changeset that introduced this regression was reviewed as 
https://reviews.llvm.org/D37955. @timshen, I'm curious to understand why you 
or'ed with `__neg_chars_.empty()` when initializing `__in_neg_chars`. The 
logic's not clear to me, and my tests seem to show that this is wrong, but I 
don't understand the reasoning behind doing it in the first place, so I might 
be introducing another bug here. Please double-check.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50534



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

[PATCH] D50543: [libcxx] Mark charconv tests as failing for previous libcxx versions.

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Ship it!


https://reviews.llvm.org/D50543



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

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.
Closed by commit rL339431: [libc++] Enable aligned allocation based on feature 
test macro, irrespective of… (authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50344?vs=159903&id=160100#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50344

Files:
  libcxx/trunk/include/__config
  libcxx/trunk/include/new
  libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+(!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__)
 #define _LIBCPP_HAS_DEFAULTRUNELOCALE
Index: libcxx/trunk/include/new
===
--- libcxx/trunk/include/new
+++ libcxx/trunk/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
+++ libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+#include 
+
+
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#   error "libc++ should have aligned allocation in C++17 and up when 
targeting a platform that supports it"
+#endif
+
+int main() { }


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -988,6 +988,11 @@
 #  endif
 #endif // defined(__APPLE__)
 
+#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+(!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__)
 #define _LIBCPP_HAS_DEFAULTRUNELOCALE
Index: libcxx/trunk/include/new
===
--- libcxx/trunk/include/new
+++ libcxx/trunk/include/new
@@ -108,13 +108,6 @@
 # define _LIBCPP_HAS_NO_SIZED_DEALLOCATION
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
-(!(defined(_LIBCPP_BUILDING_LIBRARY) || _LIBCPP_STD_VER > 14 || \
-(defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
-# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
-#endif
-
-
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
Index: libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
===
--- libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
+++ libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_

[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49240#1195125, @thakis wrote:

> When we updated out clang bundle in chromium (which includes libc++ headers), 
> our ios simulator bots regressed debug info size by ~50% due to this commit 
> (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that 
> expected?


No, this is quite surprising. What happened with the size of the Release 
builds? We should investigate, perhaps this change exposed something in Clang's 
debug info generation.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49240



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

[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49240#1195723, @rnk wrote:

> In https://reviews.llvm.org/D49240#1195237, @ldionne wrote:
>
> > In https://reviews.llvm.org/D49240#1195125, @thakis wrote:
> >
> > > When we updated out clang bundle in chromium (which includes libc++ 
> > > headers), our ios simulator bots regressed debug info size by ~50% due to 
> > > this commit 
> > > (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is 
> > > that expected?
> >
> >
> > No, this is quite surprising. What happened with the size of the Release 
> > builds? We should investigate, perhaps this change exposed something in 
> > Clang's debug info generation.
>
>
> It looks like the increase is entirely from more symbols in the symbol table. 
> It's not a problem with clang's debug info. It would help a lot if ld64 
> de-duplicated strings in the symbol table, if that's possible.
>
> [...]


Ah, thanks a lot for taking a look! Yes, this makes a lot of sense, since now 
we're not inlining everything anymore. So the code size is actually smaller 
(-9.8%), but there's more symbols because more functions are emitted. In this 
case, I would say this is expected, if unfortunate. Also, a similar effect 
would probably be witnessed if Chromium were to change their standard library 
to libstdc++, for example, since libstdc++ does not abuse inlining like libc++ 
used to.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49240



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

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160169.
ldionne added a comment.

Rewrite the patch in light of timshen's comment, and add tests.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50534

Files:
  libcxx/include/regex
  libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
  libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp


Index: libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
===
--- libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
+++ libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
@@ -18,7 +18,7 @@
 
 #include 
 #include 
-#include "test_macros.h"
+
 
 // PR34310
 int main()
Index: 
libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[X]")));
+assert(std::regex_match("X", std::regex("[XY]")));
+assert(!std::regex_match("X", std::regex("[^X]")));
+assert(!std::regex_match("X", std::regex("[^XY]")));
+
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\w]")));
+assert(std::regex_match("_", std::regex("[\\w]")));
+assert(!std::regex_match("X", std::regex("[^\\w]")));
+assert(!std::regex_match("_", std::regex("[^\\w]")));
+
+assert(!std::regex_match("X", std::regex("[\\W]")));
+assert(!std::regex_match("_", std::regex("[\\W]")));
+assert(std::regex_match("X", std::regex("[^\\W]")));
+assert(std::regex_match("_", std::regex("[^\\W]")));
+}
Index: libcxx/include/regex
===
--- libcxx/include/regex
+++ libcxx/include/regex
@@ -2414,20 +2414,17 @@
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.
+if (!(__neg_mask_ == 0 && __neg_chars_.empty()))
 {
-  const bool __in_neg_mask = (__neg_mask_ == 0) ||
-  __traits_.isctype(__ch, __neg_mask_);
+const bool __in_neg_mask = __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))


Index: libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
===
--- libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
+++ libcxx/test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
@@ -18,7 +18,7 @@
 
 #include 
 #include 
-#include "test_macros.h"
+
 
 // PR34310
 int main()
Index: libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50534#1194664, @timshen wrote:

> I'm not fully equipped with the context right now, but something doesn't add 
> up. if `__neg_chars_.empty()` check is removed, the `(__neg_mask_ == 0)` 
> above should be removed too. They have to be consistent.
>
> However, there is more weirdness in it. The comment above describes the 
> intention:
>
>   union(complement(union(__neg_chars_, __neg_mask_)), other cases...)
>   
>
> With the `__neg_chars_.empty()` and `(__neg_mask_ == 0)` removed, I believe 
> that the code exactly matches the comment. Let's see what happens when users 
> don't specify any negative class or chars. `__neg_chars_` and `__neg_mask_` 
> will be empty sets, and `union(complement(union(__neg_chars_, __neg_mask_)), 
> other cases...)` always evaluate to full set, which means it always matches 
> all characters. This can't be right.
>
> It's likely that the comment description doesn't fully describe the intended 
> behavior. I think we need to figure that out first.


Yes, I think you were right. I think what was missing is that the comment does 
not apply when there's no `neg_mask` and no `neg_chars`. Otherwise we get into 
that situation where we're taking the complement of an empty set, and we match 
everything, which is not what we want.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50534



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

[PATCH] D50341: [libcxx] Mark aligned allocation tests as XFAIL on old OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I still think we should go forward with this change since the tests _are_ 
expected to fail on the provided OS X versions, which do not contain the 
required operators.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49240#1196878, @hans wrote:

> The reason we noticed this was that it caused a *50 GB* size increase of the 
> build output on our buildbots, which was enough to cause infrastructure 
> problems.
>
> This change was also committed shortly before the 7.0 branch, so it's part of 
> the 7.0.0 release candidates.
>
> Should we back it out until a solution is found?


The thing is -- there's no solution without changing the guarantees that libc++ 
make. Today, libc++ guarantees that you can link TUs that were built with 
different versions of libc++ together. If we remove that guarantee, then we can 
use linkonce_odr and solve the problem that Chromium is having.

Is that guarantee something that Chromium is relying upon?


Repository:
  rCXX libc++

https://reviews.llvm.org/D49240



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

[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49240#1197149, @hans wrote:

> In https://reviews.llvm.org/D49240#1197052, @ldionne wrote:
>
> > In https://reviews.llvm.org/D49240#1196878, @hans wrote:
> >
> > > The reason we noticed this was that it caused a *50 GB* size increase of 
> > > the build output on our buildbots, which was enough to cause 
> > > infrastructure problems.
> > >
> > > This change was also committed shortly before the 7.0 branch, so it's 
> > > part of the 7.0.0 release candidates.
> > >
> > > Should we back it out until a solution is found?
> >
> >
> > The thing is -- there's no solution without changing the guarantees that 
> > libc++ make. Today, libc++ guarantees that you can link TUs that were built 
> > with different versions of libc++ together. If we remove that guarantee, 
> > then we can use linkonce_odr and solve the problem that Chromium is having.
> >
> > Is that guarantee something that Chromium is relying upon?
>
>
> I'm not sure (thakis can probably answer better), but isn't it enough to link 
> against some system libraries that might use libc++ for this to be true?


One would have to link statically against a system library that was statically 
linked against libc++ -- I don't think this happens, at least not on Darwin 
AFAICT. As soon as we cross a dylib boundary, we're safe because those symbols 
hidden from the ABI are not exported from the dylib. The only problem is when 
distributing static archives using different versions of libc++, where we don't 
want presumably ODR-equivalent symbols to be deduplicated (because they might 
not actually be ODR-equivalent).

> The previous solution, using inlining, while not very elegant didn't have 
> this giant binary size problem, so maybe it was better?

Just to be clear, it's only about the number of symbols, not actual code size.

> What should we put in the release notes, that folks should expect 
> significantly larger binaries using the 7.0.0 version?

If the current state is not acceptable, we should roll back the change and only 
put it in once we _also_ know how to allow ODR-deduplicating across TUs. We 
don't have a solution for that right now -- this was a follow-up step that was 
planned in the future since this one was semantics-changing. If the current 
state is acceptable, we can put in the release notes that significant symbol 
size increases should be expected using LLVM 7.0.0.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49240



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added reviewers: EricWF, mclow.lists, dexonsmith, hans, rnk.
Herald added subscribers: cfe-commits, christof.

This led to symbol size problems in Chromium, and we expect this may be
the case in other projects built in debug mode too. Instead, unless users
explicitly ask for internal_linkage, we use always_inline like we used to.

In the future, when we have a solution that allows us to drop always_inline
without falling back on internal_linkage, we can replace always_inline by
that.

Note that this commit introduces a change in contract for existing libc++
users: by default, libc++ used to guarantee that TUs built with different
versions of libc++ could be linked together. With the introduction of the
_LIBCPP_HIDE_FROM_ABI_PER_TU macro, the default behavior is that TUs built
with different libc++ versions are not guaranteed to link. This is a change
in contract but not a change in behavior, since the current implementation
still allows linking TUs built with different libc++ versions together.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652

Files:
  libcxx/docs/DesignDocs/VisibilityMacros.rst
  libcxx/include/__config


Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -796,7 +796,11 @@
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_HIDE_FROM_ABI_PER_TU
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -61,6 +61,18 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI are local to each
+  translation unit. When enabled, this means that translation units compiled
+  with different versions of libc++ can be linked together, since all non
+  ABI-facing functions are local to each translation unit. This allows static
+  archives built with different versions of libc++ to be linked together.
+
+  When the macro is not defined (the default), there is no guarantee that
+  translation units compiled with different versions of libc++ can 
interoperate.
+  However, this leads to code size improvements, since non ABI-facing functions
+  can be deduplicated across translation unit boundaries.
+
 **_LIBCPP_TYPE_VIS**
   Mark a type's typeinfo, vtable and members as having default visibility.
   This attribute cannot be used on class templates.


Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -796,7 +796,11 @@
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_HIDE_FROM_ABI_PER_TU
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -61,6 +61,18 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI are local to each
+  translation unit. When enabled, this means that translation units compiled
+  with different versions of libc++ can be linked together, since all non
+  ABI-facing functions are local to each translation unit. This allows static
+  archives built with different versions of libc++ to be linked together.
+
+  When the macro is not defined (the default), there is no guarantee that
+  translation units compiled with different versions of libc++ can interoperate.
+  However, this leads to code size improvements, since non ABI-facing functions
+  can be deduplicated across translation unit boundaries.
+
 **_LIBCPP_TYPE_VIS**
   Mark a type's typeinfo, vtable and members as having default visibility.
   This attribute cannot be used on class templates.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I opened a straw man proposal to fix this at https://reviews.llvm.org/D50652.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49240



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

The intent is for this patch to be cherry-picked onto the LLVM 7 release.

This is a straw man proposal to fix issues raised in 
https://reviews.llvm.org/D49240. The idea is that in the future, we would 
probably want the non-`internal_linkage` case to be the default. By introducing 
the `_LIBCPP_HIDE_FROM_ABI_PER_TU` setting, we're setting up libc++ users for 
the right default (i.e. no insane guarantee of being able to link TUs built 
with different libc++ versions), without actually changing any behavior for the 
LLVM 7 release. Once we have a solution that allows us to drop 
`_LIBCPP_ALWAYS_INLINE` from `_LIBCPP_HIDE_FROM_ABI` (in LLVM 8), we can just 
do it and everybody should see code size improvements, without a crazy increase 
in the number of symbols (as reported by Chromium). In LLVM8, the few projects 
that actually need to link TUs built with different libc++ versions can then 
just define `_LIBCPP_HIDE_FROM_ABI_PER_TU` to keep today's behavior.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160414.
ldionne added a comment.

Update documentation for _LIBCPP_HIDE_FROM_ABI


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652

Files:
  libcxx/docs/DesignDocs/VisibilityMacros.rst
  libcxx/include/__config


Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -796,7 +796,11 @@
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_HIDE_FROM_ABI_PER_TU
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,21 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI with 
`_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. When enabled, this means that translation units compiled
+  with different versions of libc++ can be linked together, since all non
+  ABI-facing functions are local to each translation unit. This allows static
+  archives built with different versions of libc++ to be linked together. This
+  also means that functions marked with `_LIBCPP_HIDE_FROM_ABI` are not 
guaranteed
+  to have the same address across translation unit boundaries.
+
+  When the macro is not defined (the default), there is no guarantee that
+  translation units compiled with different versions of libc++ can 
interoperate.
+  However, this leads to code size improvements, since non ABI-facing functions
+  can be deduplicated across translation unit boundaries.
+
 **_LIBCPP_TYPE_VIS**
   Mark a type's typeinfo, vtable and members as having default visibility.
   This attribute cannot be used on class templates.


Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -796,7 +796,11 @@
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_HIDE_FROM_ABI_PER_TU
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,21 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI with `_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. When enabled, this means that translation units compiled
+  with different versions of libc++ can be linked together, since all non
+  ABI-facing functions are local to each translation unit. This allows static
+  archives built with different versions of libc++ to be linked together. This
+  also means that functions marked with `_LIBCPP_HIDE_FROM_ABI` are not guaranteed
+  to have the same address across translation unit boundaries.
+
+  When the macro is not defined (the default), there is no guarantee that
+  translation units compiled with different versions of libc++ can interoperate.
+  However, this le

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1197775, @rnk wrote:

> I'd prefer not to do this, since internal_linkage generates smaller, more 
> debuggable code by default. I think the symbol table size increase may be 
> specific to MachO, and it may be possible to work around this by changing 
> ld64 to pool strings for symbols by default. I don't know enough about MachO 
> to say if this is possible.
>
> I also think the symbol table size problem may be limited to "large" users of 
> C++: people with 500+ object files using libc++ in a DSO. I'm more 
> comfortable imposing a cost on these users to ask them to opt in to some 
> setting that disables internal_linkage and always_inline than I am leaving 
> always_inline on by default, which adversely affects all users.


One thing to keep in mind is that we do not have a solution that allows 
removing both `internal_linkage` and `always_inline`. It's either 
`internal_linkage` or `always_inline`, but you can't get rid of both until we 
fix some problems with extern template declarations and visibility attributes. 
What I can do, however, is reverse the default to use `internal_linkage`, and 
then have a temporary hook that allows Chromium to stay on `always_inline`. In 
the future, we'd remove that hook and the choice would be between 
`internal_linkage` and nothing at all.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50341: [libcxx] Mark aligned allocation tests as XFAIL on old OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160440.
ldionne added a comment.

Rewrite all XFAILs in light of issues brought up by Marshall.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341

Files:
  libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp

Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
@@ -15,11 +15,12 @@
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
@@ -10,16 +10,17 @@
 
 // 
 
-// void* operator new[](std::size_t, std::align_val_t);
+// void* operator new(std::size_t, std::align_val_t);
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new[](4, std::align_val_t{4});  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4});  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// -*- C++ -*-
-//===--===//
-//
-// 

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

This works with all the dylibs I have locally, with various combinations of 
availability enabled/disabled and all standards. I've asked Marshall to check 
on his system, where he initially reported some failures.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1197830, @rnk wrote:

> In https://reviews.llvm.org/D50652#1197780, @ldionne wrote:
>
> > One thing to keep in mind is that we do not have a solution that allows 
> > removing both `internal_linkage` and `always_inline`. It's either 
> > `internal_linkage` or `always_inline`, but you can't get rid of both until 
> > we fix some problems with extern template declarations and visibility 
> > attributes. What I can do, however, is reverse the default to use 
> > `internal_linkage`, and then have a temporary hook that allows Chromium to 
> > stay on `always_inline`. In the future, we'd remove that hook and the 
> > choice would be between `internal_linkage` and nothing at all.
>
>
> It's probably worth it to me to debug and understand that problem. Is there a 
> good explanation of it?


http://lists.llvm.org/pipermail/llvm-dev/2018-July/124549.html

This is understood, it just needs more work than we can reasonably expect to 
get into LLVM 7.

> Also, if a user could define _LIBCPP_ABI_UNSTABLE, does this problem still 
> apply? Could we remove the problematic visibility attributes if the ABI is 
> unstable?

I guess in that case, we could simply remove _all_ visibility attributes (and 
linkage attributes). libc++ may end up exporting more symbols than we want, but 
defining `_LIBCPP_ABI_UNSTABLE` would explicitly state that we don't care about 
that. Doing this is a possibility, but don't think this is the way we want to 
solve the problem at hand.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160457.
ldionne added a comment.

Switch from XFAIL to UNSUPPORTED, per Marshall's offline comment.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341

Files:
  libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp

Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
@@ -15,11 +15,12 @@
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
@@ -10,16 +10,17 @@
 
 // 
 
-// void* operator new[](std::size_t, std::align_val_t);
+// void* operator new(std::size_t, std::align_val_t);
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new[](4, std::align_val_t{4});  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4});  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// -*- C++ -*-
-//===--===//
-//
-// 

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160465.
ldionne marked an inline comment as done.
ldionne added a comment.

Replace more XFAILs by UNSUPPORTEDs, per Marshall's comment.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341

Files:
  libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
  
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp

Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp
@@ -15,11 +15,12 @@
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4}, std::nothrow);  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
+++ libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
@@ -10,16 +10,17 @@
 
 // 
 
-// void* operator new[](std::size_t, std::align_val_t);
+// void* operator new(std::size_t, std::align_val_t);
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, clang-3.8
 
-#include 
+// REQUIRES: -faligned-allocation
+// RUN: %compile %verify -faligned-allocation
 
-#include "test_macros.h"
+#include 
 
 int main ()
 {
-::operator new[](4, std::align_val_t{4});  // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}}
+::operator new(4, std::align_val_t{4});  // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
 }
Index: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
===
--- libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// -*- C++ -*-
-//===---

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp:18
+// XFAIL: macosx10.8
+// XFAIL: macosx10.7
 

mclow.lists wrote:
> These should probably be `UNSUPPORTED` as well.
I think the correct thing for those is `XFAIL`, since this test is actually 
testing that the macro is defined or not defined for specific platforms. 
Equivalently, we could have two tests -- one 
`aligned_allocation_macro.pass.cpp` which is `UNSUPPORTED` on `macosx10.7` ... 
`macosx10.12`, and one `aligned_allocation_macro.fail.cpp` which `REQUIRES` one 
of `macosx10.7`, ..., `macosx10.12`. Does that make sense?



Comment at: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp:15
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8

mclow.lists wrote:
> ldionne wrote:
> > vsapsai wrote:
> > > In what cases are we supposed to run these tests? Such extensive 
> > > collection of unsupported C++ standards looks suspicious and I guess that 
> > > is the reason why I haven't seen test failures with older libc++ dylibs.
> > That's an excellent question. I would assume those should be enabled in 
> > C++17 and above, I'm not sure why they're disabled. @mclow.lists was the 
> > one to introduce those tests, perhaps he can shed light on why they were 
> > disabled in C++17?
> IIRC, this call - `operator new(std::size_t, std::align_val_t);` was 
> introduced for C++17, and `[[nodiscard]]` was added for C++20.
> 
> this test is making sure that we get a warning when you call this and don't 
> use the return value. It requires C++20 and above, hence the massive 
> UNSUPPORTED list
Wouldn't it be sufficient to list the standard required? Why do we have 
`UNSUPPORTED` compilers too?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50674: [libc++] Add missing #include in C11 features tests

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: mclow.lists.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

These #includes are quite important, since otherwise any

  #if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)

checks are always false, and so we don't actually test for C11 support
in the standard library.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50674

Files:
  libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
  libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
  libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp


Index: libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
===
--- libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
+++ libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
+++ libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
===
--- libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
+++ libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
@@ -14,6 +14,9 @@
 // _LIBCPP_HAS_C11_FEATURES - which is defined in <__config>
 // They should always be the same
 
+#include <__config>
+#include "test_macros.h"
+
 #ifdef TEST_HAS_C11_FEATURES
 # ifndef _LIBCPP_HAS_C11_FEATURES
 #  error "TEST_HAS_C11_FEATURES is defined, but _LIBCPP_HAS_C11_FEATURES is 
not"


Index: libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
===
--- libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
+++ libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
+++ libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
===
--- libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
+++ libcxx/test/libcxx/language.support/has_c11_features.pass.cpp
@@ -14,6 +14,9 @@
 //		_LIBCPP_HAS_C11_FEATURES - which is defined in <__config>
 //	They should always be the same
 
+#include <__config>
+#include "test_macros.h"
+
 #ifdef TEST_HAS_C11_FEATURES
 # ifndef _LIBCPP_HAS_C11_FEATURES
 #  error "TEST_HAS_C11_FEATURES is defined, but _LIBCPP_HAS_C11_FEATURES is not"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50341#1198339, @vsapsai wrote:

> What about defining a feature for unsupported configurations? I've tried
>
>   if '__cpp_aligned_new' not in macros or \
>   intMacroValue(macros['__cpp_aligned_new']) < 201606:
>   self.config.available_features.add('libcpp-no-aligned-new')
>   
>
> and `// UNSUPPORTED: libcpp-no-aligned-new`. Seems to be working but it's not 
> sufficient. For example, clang-6 for old macOS versions would define 
> `__cpp_aligned_new` but a test would fail. Still, we can use another feature, 
> not necessarily macro-based. Though probably something like 
> `libcpp-no-aligned-new` can replace `// UNSUPPORTED: c++98, c++03, c++11, 
> c++14`.


I thought about something like this, but it would only be useful in a couple of 
places, and I still don't understand why the tests are marked as unsupported on 
some compilers at all. Also, there's already a feature called 
`no-aligned-alloc`, which tests whether `-faligned-alloc` is supported, so we 
should avoid confusing both.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50674: [libc++] Add missing #include in C11 features tests

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339675: [libc++] Add missing #include in C11 features tests 
(authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50674?vs=160475&id=160567#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50674

Files:
  libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
  libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
  libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp


Index: 
libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
===
--- 
libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
+++ 
libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
===
--- libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
+++ libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
===
--- libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
+++ libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
@@ -14,6 +14,9 @@
 // _LIBCPP_HAS_C11_FEATURES - which is defined in <__config>
 // They should always be the same
 
+#include <__config>
+#include "test_macros.h"
+
 #ifdef TEST_HAS_C11_FEATURES
 # ifndef _LIBCPP_HAS_C11_FEATURES
 #  error "TEST_HAS_C11_FEATURES is defined, but _LIBCPP_HAS_C11_FEATURES is 
not"


Index: libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
===
--- libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
+++ libcxx/trunk/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
===
--- libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
+++ libcxx/trunk/test/std/depr/depr.c.headers/float_h.pass.cpp
@@ -11,6 +11,8 @@
 
 #include 
 
+#include "test_macros.h"
+
 #ifndef FLT_ROUNDS
 #error FLT_ROUNDS not defined
 #endif
Index: libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
===
--- libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
+++ libcxx/trunk/test/libcxx/language.support/has_c11_features.pass.cpp
@@ -14,6 +14,9 @@
 //		_LIBCPP_HAS_C11_FEATURES - which is defined in <__config>
 //	They should always be the same
 
+#include <__config>
+#include "test_macros.h"
+
 #ifdef TEST_HAS_C11_FEATURES
 # ifndef _LIBCPP_HAS_C11_FEATURES
 #  error "TEST_HAS_C11_FEATURES is defined, but _LIBCPP_HAS_C11_FEATURES is not"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1198885, @hans wrote:

> Oh, or could we do
>
>   -D_LIBCPP_HIDE_FROM_ABI=
>
>
> and just get regular odr linkage for these functions?


No, you do need to use `_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` because of the 
issue described in 
http://lists.llvm.org/pipermail/llvm-dev/2018-July/124549.html. But yeah, 
Chromium could use this workaround.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1198893, @ldionne wrote:

> In https://reviews.llvm.org/D50652#1198885, @hans wrote:
>
> > Oh, or could we do
> >
> >   -D_LIBCPP_HIDE_FROM_ABI=
> >
> >
> > and just get regular odr linkage for these functions?
>
>
> No, you do need to use `_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` because of the 
> issue described in 
> http://lists.llvm.org/pipermail/llvm-dev/2018-July/124549.html. But yeah, 
> Chromium could use this workaround.


Actually, scratch that, it does work. One can either use 
`-D_LIBCPP_HIDE_FROM_ABI=_LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE` to restore the 
old behavior, or `-D_LIBCPP_HIDE_FROM_ABI=` to get odr linkage.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: mclow.lists.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

The macro was not defined in C++11 mode when it should have been, at least
according to how _LIBCPP_HAS_C11_FEATURES is defined.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719

Files:
  libcxx/test/support/test_macros.h


Index: libcxx/test/support/test_macros.h
===
--- libcxx/test/support/test_macros.h
+++ libcxx/test/support/test_macros.h
@@ -124,7 +124,7 @@
 
 // Sniff out to see if the underling C library has C11 features
 // Note that at this time (July 2018), MacOS X and iOS do NOT.
-#if __ISO_C_VISIBLE >= 2011
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
 #  if defined(__FreeBSD__)
 #define TEST_HAS_C11_FEATURES
 #  elif defined(__Fuchsia__)


Index: libcxx/test/support/test_macros.h
===
--- libcxx/test/support/test_macros.h
+++ libcxx/test/support/test_macros.h
@@ -124,7 +124,7 @@
 
 // Sniff out to see if the underling C library has C11 features
 // Note that at this time (July 2018), MacOS X and iOS do NOT.
-#if __ISO_C_VISIBLE >= 2011
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
 #  if defined(__FreeBSD__)
 #define TEST_HAS_C11_FEATURES
 #  elif defined(__Fuchsia__)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

The goal of this commit is to fix build bot failures here: 
http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-armv7-linux-noexceptions/builds/286/steps/test.libcxx/logs/stdio.
 This failure was introduced when I fixed some unit tests that were no-ops in 
https://reviews.llvm.org/D50674 -- I didn't test that change on Linux so I 
didn't catch the failure before the bots notified me.

To test this change, I bootstrapped a Docker image with a configuration similar 
to the build bot, and was able to reproduce the issue. Applying this patch 
fixed the issue.

Side note: we should probably strive to have docker containers for as many 
libc++ build bots as possible -- this makes it possible to reproduce failures 
locally, which is otherwise a guessing game. Even better -- we could run better 
tests before submitting a change and avoid breaking the build altogether!


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719



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

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50719#1199427, @mclow.lists wrote:

> I have pissed in this area, too - See 
> https://bugs.llvm.org/show_bug.cgi?id=38495 and the proposed resolution here: 
> https://bugs.llvm.org/attachment.cgi?id=20692
>  How about I just make this change as part of that fix?


Yes, that would be great.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719



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

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Louis Dionne via Phabricator via cfe-commits]

ldionne marked an inline comment as done.
ldionne added a comment.

Ok, I'm pushing with JF's suggested change (use `TEST_STD_VER >= 11` instead of 
`__cplusplus >= 201103L`). Let's cross fingers that this is going to unbreak 
the testers -- like I said it fixed my Docker container.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719



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

[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX339702: [libc++] Fix incorrect definition of 
TEST_HAS_C11_FEATURES (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50719?vs=160616&id=160648#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D50719

Files:
  test/support/test_macros.h


Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -124,7 +124,7 @@
 
 // Sniff out to see if the underling C library has C11 features
 // Note that at this time (July 2018), MacOS X and iOS do NOT.
-#if __ISO_C_VISIBLE >= 2011
+#if __ISO_C_VISIBLE >= 2011 || TEST_STD_VER >= 11
 #  if defined(__FreeBSD__)
 #define TEST_HAS_C11_FEATURES
 #  elif defined(__Fuchsia__)


Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -124,7 +124,7 @@
 
 // Sniff out to see if the underling C library has C11 features
 // Note that at this time (July 2018), MacOS X and iOS do NOT.
-#if __ISO_C_VISIBLE >= 2011
+#if __ISO_C_VISIBLE >= 2011 || TEST_STD_VER >= 11
 #  if defined(__FreeBSD__)
 #define TEST_HAS_C11_FEATURES
 #  elif defined(__Fuchsia__)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50341#1198863, @ldionne wrote:

> In https://reviews.llvm.org/D50341#1198339, @vsapsai wrote:
>
> > What about defining a feature for unsupported configurations? I've tried
> >
> >   if '__cpp_aligned_new' not in macros or \
> >   intMacroValue(macros['__cpp_aligned_new']) < 201606:
> >   self.config.available_features.add('libcpp-no-aligned-new')
> >   
> >
> > and `// UNSUPPORTED: libcpp-no-aligned-new`. Seems to be working but it's 
> > not sufficient. For example, clang-6 for old macOS versions would define 
> > `__cpp_aligned_new` but a test would fail. Still, we can use another 
> > feature, not necessarily macro-based. Though probably something like 
> > `libcpp-no-aligned-new` can replace `// UNSUPPORTED: c++98, c++03, c++11, 
> > c++14`.
>
>
> I thought about something like this, but it would only be useful in a couple 
> of places, and I still don't understand why the tests are marked as 
> unsupported on some compilers at all. Also, there's already a feature called 
> `no-aligned-alloc`, which tests whether `-faligned-alloc` is supported, so we 
> should avoid confusing both.


After discussing with Volodymyr, it's not clear exactly how we might create a 
`libcpp-no-aligned-new` check that properly expresses when aligned allocations 
are available or not. It also seems like there are no other lit features based 
on such a combination of compiler/target platform/standard/system_cxx_lib. I 
think we're better off just keeping the duplication of `UNSUPPORTED` cases 
right now, and we can revisit this idea in the future if we find ourselves 
needing that feature again.

Is there any blocking comment on this patch? I'd like to get it through to see 
if it fixes our testers (and Marshall's local testing, too).


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160680.
ldionne added a comment.
Herald added a subscriber: mgorny.

Add a way to change the default behavior of _LIBCPP_HIDE_FROM_ABI at build 
time. Also, rename the macro to _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE to 
align with other ABI-related macros.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652

Files:
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxx/docs/DesignDocs/VisibilityMacros.rst
  libcxx/include/__config
  libcxx/include/__config_site.in

Index: libcxx/include/__config_site.in
===
--- libcxx/include/__config_site.in
+++ libcxx/include/__config_site.in
@@ -14,6 +14,7 @@
 #cmakedefine _LIBCPP_ABI_UNSTABLE
 #cmakedefine _LIBCPP_ABI_FORCE_ITANIUM
 #cmakedefine _LIBCPP_ABI_FORCE_MICROSOFT
+#cmakedefine _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE
 #cmakedefine _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE
 #cmakedefine _LIBCPP_HAS_NO_STDIN
 #cmakedefine _LIBCPP_HAS_NO_STDOUT
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -796,7 +796,11 @@
 #endif
 
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,21 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE**
+  This macro controls whether symbols hidden from the ABI with `_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. When enabled, this means that translation units compiled
+  with different versions of libc++ can be linked together, since all non
+  ABI-facing functions are local to each translation unit. This allows static
+  archives built with different versions of libc++ to be linked together. This
+  also means that functions marked with `_LIBCPP_HIDE_FROM_ABI` are not guaranteed
+  to have the same address across translation unit boundaries.
+
+  When the macro is not defined (the default), there is no guarantee that
+  translation units compiled with different versions of libc++ can interoperate.
+  However, this leads to code size improvements, since non ABI-facing functions
+  can be deduplicated across translation unit boundaries.
+
 **_LIBCPP_TYPE_VIS**
   Mark a type's typeinfo, vtable and members as having default visibility.
   This attribute cannot be used on class templates.
Index: libcxx/docs/BuildingLibcxx.rst
===
--- libcxx/docs/BuildingLibcxx.rst
+++ libcxx/docs/BuildingLibcxx.rst
@@ -351,6 +351,15 @@
   Build the "unstable" ABI version of libc++. Includes all ABI changing features
   on top of the current stable version.
 
+.. option:: LIBCXX_ABI_HIDDEN_USE_INTERNAL_LINKAGE:BOOL
+
+  **Default**: ``OFF``
+
+  Give internal linkage to all symbols that are not part of the ABI. This
+  allows linking translation units built with different versions of libc++
+  together, at the cost of code bloat (because non exported functions are
+  duplicated in each TU).
+
 .. option:: LIBCXX_ABI_DEFINES:STRING
 
   **Default**: ``""``
Index: libcxx/CMakeLists.txt
===
--- libcxx/CMakeLists.txt
+++ libcxx/CMakeLists.txt
@@ -120,6 +120,7 @@
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the Microsoft ABI.")
+option(LIBCXX_ABI_HIDDEN_USE_INTERNAL_LINKAGE "Give internal linkage to symbols that are not part of the ABI" OFF)
 set(LIBCXX_ABI_DEFINES "" CACHE STRING "A semicolon separated list of ABI macros to define in the site config header.")
 option(LIBCXX_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF

[PATCH] D50739: Clean up macros to detect underling C library functionality

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: include/__config:433
 
-#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
+#if __ISO_C_VISIBLE >= 2011
 #  if defined(__FreeBSD__)

mclow.lists wrote:
> Should we be using `__ISO_C_VISIBLE` here, or `__STDC_VERSION__`?
> I didn't change this, but ...
The weird part is that neither is defined when compiling C++ code..


https://reviews.llvm.org/D50739



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

[PATCH] D50739: Clean up macros to detect underling C library functionality

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: include/__config:433
 
-#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
+#if __ISO_C_VISIBLE >= 2011
 #  if defined(__FreeBSD__)

ldionne wrote:
> mclow.lists wrote:
> > Should we be using `__ISO_C_VISIBLE` here, or `__STDC_VERSION__`?
> > I didn't change this, but ...
> The weird part is that neither is defined when compiling C++ code..
This means that `TEST_HAS_C11_FEATURES` and `_LIBCPP_HAS_C11_FEATURES` will 
both be disabled all the time.


https://reviews.llvm.org/D50739



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

[PATCH] D50748: [libc++] Detect C11 features on non-Clang compilers

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: mclow.lists.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof, krytarowski.

The macros were inside `#if defined(_LIBCPP_COMPILER_CLANG)`, which means
we would never detect C11 features on non-Clang compilers. According to
Marshall Clow, this is not the intended behavior.

[libc++] Disable failing C11 feature tests for  and 

Those tests are breaking the test bots. A Bugzilla has been filed to
make sure those tests are re-enabled: 
https://bugs.llvm.org/show_bug.cgi?id=38572


Repository:
  rCXX libc++

https://reviews.llvm.org/D50748

Files:
  libcxx/include/__config
  libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
  libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp

Index: libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
===
--- libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
+++ libcxx/test/std/language.support/support.limits/c.limits/cfloat.pass.cpp
@@ -25,7 +25,7 @@
 #error FLT_RADIX not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_HAS_SUBNORM
 #error FLT_HAS_SUBNORM not defined
 #endif
@@ -55,7 +55,7 @@
 #error DECIMAL_DIG not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_DECIMAL_DIG
 #error FLT_DECIMAL_DIG not defined
 #endif
@@ -165,7 +165,7 @@
 #error LDBL_MIN not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_TRUE_MIN
 #error FLT_TRUE_MIN not defined
 #endif
Index: libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
===
--- libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
+++ libcxx/test/std/depr/depr.c.headers/float_h.pass.cpp
@@ -25,7 +25,7 @@
 #error FLT_RADIX not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_HAS_SUBNORM
 #error FLT_HAS_SUBNORM not defined
 #endif
@@ -55,7 +55,7 @@
 #error DECIMAL_DIG not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_DECIMAL_DIG
 #error FLT_DECIMAL_DIG not defined
 #endif
@@ -165,7 +165,7 @@
 #error LDBL_MIN not defined
 #endif
 
-#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES)
+#if TEST_STD_VER > 14 && defined(TEST_HAS_C11_FEATURES) && 0
 #ifndef FLT_TRUE_MIN
 #error FLT_TRUE_MIN not defined
 #endif
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -328,6 +328,28 @@
 #  define _LIBCPP_NO_CFI
 #endif
 
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
+#  if defined(__FreeBSD__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__Fuchsia__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__linux__)
+#if !defined(_LIBCPP_HAS_MUSL_LIBC)
+#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#  endif
+#  if _LIBCPP_GLIBC_PREREQ(2, 17)
+#define _LIBCPP_HAS_C11_FEATURES
+#  endif
+#else // defined(_LIBCPP_HAS_MUSL_LIBC)
+#  define _LIBCPP_HAS_QUICK_EXIT
+#  define _LIBCPP_HAS_C11_FEATURES
+#endif
+#  endif // __linux__
+#endif
+
 #if defined(_LIBCPP_COMPILER_CLANG)
 
 // _LIBCPP_ALTERNATE_STRING_LAYOUT is an old name for
@@ -430,28 +452,6 @@
 #define _LIBCPP_HAS_NO_VARIABLE_TEMPLATES
 #endif
 
-#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
-#  if defined(__FreeBSD__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__Fuchsia__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__linux__)
-#if !defined(_LIBCPP_HAS_MUSL_LIBC)
-#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#  endif
-#  if _LIBCPP_GLIBC_PREREQ(2, 17)
-#define _LIBCPP_HAS_C11_FEATURES
-#  endif
-#else // defined(_LIBCPP_HAS_MUSL_LIBC)
-#  define _LIBCPP_HAS_QUICK_EXIT
-#  define _LIBCPP_HAS_C11_FEATURES
-#endif
-#  endif // __linux__
-#endif
-
 #if !(__has_feature(cxx_noexcept))
 #define _LIBCPP_HAS_NO_NOEXCEPT
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50748: [libc++] Detect C11 features on non-Clang compilers

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX339741: [libc++] Detect C11 features on non-Clang 
compilers (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50748?vs=160725&id=160727#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D50748

Files:
  include/__config


Index: include/__config
===
--- include/__config
+++ include/__config
@@ -328,6 +328,28 @@
 #  define _LIBCPP_NO_CFI
 #endif
 
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
+#  if defined(__FreeBSD__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__Fuchsia__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__linux__)
+#if !defined(_LIBCPP_HAS_MUSL_LIBC)
+#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#  endif
+#  if _LIBCPP_GLIBC_PREREQ(2, 17)
+#define _LIBCPP_HAS_C11_FEATURES
+#  endif
+#else // defined(_LIBCPP_HAS_MUSL_LIBC)
+#  define _LIBCPP_HAS_QUICK_EXIT
+#  define _LIBCPP_HAS_C11_FEATURES
+#endif
+#  endif // __linux__
+#endif
+
 #if defined(_LIBCPP_COMPILER_CLANG)
 
 // _LIBCPP_ALTERNATE_STRING_LAYOUT is an old name for
@@ -430,28 +452,6 @@
 #define _LIBCPP_HAS_NO_VARIABLE_TEMPLATES
 #endif
 
-#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
-#  if defined(__FreeBSD__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__Fuchsia__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__linux__)
-#if !defined(_LIBCPP_HAS_MUSL_LIBC)
-#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#  endif
-#  if _LIBCPP_GLIBC_PREREQ(2, 17)
-#define _LIBCPP_HAS_C11_FEATURES
-#  endif
-#else // defined(_LIBCPP_HAS_MUSL_LIBC)
-#  define _LIBCPP_HAS_QUICK_EXIT
-#  define _LIBCPP_HAS_C11_FEATURES
-#endif
-#  endif // __linux__
-#endif
-
 #if !(__has_feature(cxx_noexcept))
 #define _LIBCPP_HAS_NO_NOEXCEPT
 #endif


Index: include/__config
===
--- include/__config
+++ include/__config
@@ -328,6 +328,28 @@
 #  define _LIBCPP_NO_CFI
 #endif
 
+#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
+#  if defined(__FreeBSD__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__Fuchsia__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#define _LIBCPP_HAS_C11_FEATURES
+#  elif defined(__linux__)
+#if !defined(_LIBCPP_HAS_MUSL_LIBC)
+#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
+#define _LIBCPP_HAS_QUICK_EXIT
+#  endif
+#  if _LIBCPP_GLIBC_PREREQ(2, 17)
+#define _LIBCPP_HAS_C11_FEATURES
+#  endif
+#else // defined(_LIBCPP_HAS_MUSL_LIBC)
+#  define _LIBCPP_HAS_QUICK_EXIT
+#  define _LIBCPP_HAS_C11_FEATURES
+#endif
+#  endif // __linux__
+#endif
+
 #if defined(_LIBCPP_COMPILER_CLANG)
 
 // _LIBCPP_ALTERNATE_STRING_LAYOUT is an old name for
@@ -430,28 +452,6 @@
 #define _LIBCPP_HAS_NO_VARIABLE_TEMPLATES
 #endif
 
-#if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
-#  if defined(__FreeBSD__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__Fuchsia__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#define _LIBCPP_HAS_C11_FEATURES
-#  elif defined(__linux__)
-#if !defined(_LIBCPP_HAS_MUSL_LIBC)
-#  if _LIBCPP_GLIBC_PREREQ(2, 15) || defined(__BIONIC__)
-#define _LIBCPP_HAS_QUICK_EXIT
-#  endif
-#  if _LIBCPP_GLIBC_PREREQ(2, 17)
-#define _LIBCPP_HAS_C11_FEATURES
-#  endif
-#else // defined(_LIBCPP_HAS_MUSL_LIBC)
-#  define _LIBCPP_HAS_QUICK_EXIT
-#  define _LIBCPP_HAS_C11_FEATURES
-#endif
-#  endif // __linux__
-#endif
-
 #if !(__has_feature(cxx_noexcept))
 #define _LIBCPP_HAS_NO_NOEXCEPT
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339743: [libcxx] Fix XFAILs for aligned allocation tests on 
older OSX versions (authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50341?vs=160465&id=160731#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50341

Files:
  libcxx/trunk/test/libcxx/memory/aligned_allocation_macro.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.sh.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.sh.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align_nothrow.sh.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_nothrow.sh.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align.sh.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.fail.cpp
  
libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_size_align_nothrow.sh.cpp

Index: libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
===
--- libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
+++ libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
@@ -9,12 +9,22 @@
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14
 
-// XFAIL: with_system_cxx_lib=macosx10.12
-// XFAIL: with_system_cxx_lib=macosx10.11
-// XFAIL: with_system_cxx_lib=macosx10.10
-// XFAIL: with_system_cxx_lib=macosx10.9
-// XFAIL: with_system_cxx_lib=macosx10.7
-// XFAIL: with_system_cxx_lib=macosx10.8
+// dylibs shipped before macosx10.13 do not provide aligned allocation, so that's a link error
+// UNSUPPORTED: with_system_cxx_lib=macosx10.12
+// UNSUPPORTED: with_system_cxx_lib=macosx10.11
+// UNSUPPORTED: with_system_cxx_lib=macosx10.10
+// UNSUPPORTED: with_system_cxx_lib=macosx10.9
+// UNSUPPORTED: with_system_cxx_lib=macosx10.8
+// UNSUPPORTED: with_system_cxx_lib=macosx10.7
+
+// Using aligned allocation functions is a compiler error when deploying to
+// platforms older than macosx10.13
+// UNSUPPORTED: macosx10.12
+// UNSUPPORTED: macosx10.11
+// UNSUPPORTED: macosx10.10
+// UNSUPPORTED: macosx10.9
+// UNSUPPORTED: macosx10.8
+// UNSUPPORTED: macosx10.7
 
 // asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
Index: libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
===
--- libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
+++ libcxx/trunk/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp
@@ -10,12 +10,24 @@
 // UNSUPPORTED: c++98, c++03, c++11, c++14
 // UNSUPPORTED: sanitizer-new-delete
 
-// XFAIL: with_system_cxx_lib=macosx10.12
-// XFAIL: with_system_cxx_lib=macosx10.11
-// XFAIL: with_system_cxx_lib=macosx10.10
-// XFAIL: with_system_

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 160873.
ldionne added a comment.

Allow picking a custom default behavior for vendors, per Duncan's comment.

Also, revert to a better name for the macro.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652

Files:
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxx/docs/DesignDocs/VisibilityMacros.rst
  libcxx/include/__config
  libcxx/include/__config_site.in
  libcxx/utils/libcxx/test/config.py

Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -677,7 +677,8 @@
 if feature_macros[m]:
 define += '=%s' % (feature_macros[m])
 self.cxx.compile_flags += [define]
-if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS':
+if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS' or \
+   m == '_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT':
 continue
 if m == '_LIBCPP_ABI_VERSION':
 self.config.available_features.add('libcpp-abi-version-v%s'
Index: libcxx/include/__config_site.in
===
--- libcxx/include/__config_site.in
+++ libcxx/include/__config_site.in
@@ -14,6 +14,7 @@
 #cmakedefine _LIBCPP_ABI_UNSTABLE
 #cmakedefine _LIBCPP_ABI_FORCE_ITANIUM
 #cmakedefine _LIBCPP_ABI_FORCE_MICROSOFT
+#cmakedefine _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT
 #cmakedefine _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE
 #cmakedefine _LIBCPP_HAS_NO_STDIN
 #cmakedefine _LIBCPP_HAS_NO_STDOUT
Index: libcxx/include/__config
===
--- libcxx/include/__config
+++ libcxx/include/__config
@@ -795,8 +795,20 @@
 #  define _LIBCPP_INTERNAL_LINKAGE _LIBCPP_ALWAYS_INLINE
 #endif
 
+#ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU
+#  ifndef _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT
+#define _LIBCPP_HIDE_FROM_ABI_PER_TU 0
+#  else
+#define _LIBCPP_HIDE_FROM_ABI_PER_TU 1
+#  endif
+#endif
+
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  if _LIBCPP_HIDE_FROM_ABI_PER_TU
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif
 
 #ifdef _LIBCPP_BUILDING_LIBRARY
Index: libcxx/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,41 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI with `_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. This macro is defined to either 0 or 1. When it is defined to
+  1, translation units compiled with different versions of libc++ can be linked
+  together, since all non ABI-facing functions are local to each translation unit.
+  This allows static archives built with different versions of libc++ to be linked
+  together. This also means that functions marked with `_LIBCPP_HIDE_FROM_ABI`
+  are not guaranteed to have the same address across translation unit boundaries.
+
+  When the macro is defined to 0, there is no guarantee that translation units
+  compiled with different versions of libc++ can interoperate. However, this
+  leads to code size improvements, since non ABI-facing functions can be
+  deduplicated across translation unit boundaries.
+
+  This macro can be defined by users to control the behavior they want from
+  libc++. The default value of this macro (0 or 1) is controlled by whether
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` is defined, which is intended to
+  be used by vendors only (see below).
+
+**_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT**
+  This macro controls the default value for `_LIBCPP_HIDE_FROM_ABI_PER_TU`.
+  When the macro is defined, per TU ABI insulation is enabled by default, and
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU` is defined to 1 unless overriden by users.
+  Otherwise, per TU ABI insulation is disabled by default, and
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU` is defined to 0 unless overriden by users.
+
+  This mac

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1201236, @thakis wrote:

> I haven't read all the messages in these threads, forgive me if someone asked 
> this already. It's a bit weird to me that we have to override this behavior 
> in Chromium while the default is different. Why isn't the executable size 
> blowup we see in chromium a problem for everyone else too? Is the plan to fix 
> ld64's string pooling at the same time as rolling this change out, and this 
> is just a workaround for people who have head libc++ but not head ld64?


It's the other way around -- the default behavior introduced in this patch is 
the one before the `internal_linkage` change. If you want to opt-in, you can 
define `_LIBCPP_HIDE_FROM_ABI_PER_TU`.

As a separate goal, we will (in the future) make things better for the default 
case, i.e. we will get rid of `__always_inline__` too and allow 
ODR-deduplication.

Does that make sense?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50799: Fix for PR 38495: no longer compiles on FreeBSD, due to lack of timespec_get()

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM.


https://reviews.llvm.org/D50799



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

If that's possible at all, I would like for the Chromium people to build with 
this patch applied. The expectation is that we'll cherry-pick this patch onto 
LLVM 7, and it would suck if that did not solve Chromium's problem for some 
stupid reason.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50815: Establish the header

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D50815



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339874: [libcxx] By default, do not use internal_linkage to 
hide symbols from the ABI (authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50652?vs=160873&id=161009#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50652

Files:
  libcxx/trunk/CMakeLists.txt
  libcxx/trunk/docs/BuildingLibcxx.rst
  libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
  libcxx/trunk/include/__config
  libcxx/trunk/include/__config_site.in
  libcxx/trunk/utils/libcxx/test/config.py

Index: libcxx/trunk/CMakeLists.txt
===
--- libcxx/trunk/CMakeLists.txt
+++ libcxx/trunk/CMakeLists.txt
@@ -120,6 +120,7 @@
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the Microsoft ABI.")
+option(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT "Enable per TU ABI insulation by default. To be used by vendors." OFF)
 set(LIBCXX_ABI_DEFINES "" CACHE STRING "A semicolon separated list of ABI macros to define in the site config header.")
 option(LIBCXX_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 
@@ -662,6 +663,7 @@
 config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)
 config_define_if(LIBCXX_ABI_FORCE_ITANIUM _LIBCPP_ABI_FORCE_ITANIUM)
 config_define_if(LIBCXX_ABI_FORCE_MICROSOFT _LIBCPP_ABI_FORCE_MICROSOFT)
+config_define_if(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT _LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT)
 
 config_define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
 config_define_if_not(LIBCXX_ENABLE_STDIN _LIBCPP_HAS_NO_STDIN)
Index: libcxx/trunk/utils/libcxx/test/config.py
===
--- libcxx/trunk/utils/libcxx/test/config.py
+++ libcxx/trunk/utils/libcxx/test/config.py
@@ -677,7 +677,8 @@
 if feature_macros[m]:
 define += '=%s' % (feature_macros[m])
 self.cxx.compile_flags += [define]
-if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS':
+if m == '_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS' or \
+   m == '_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT':
 continue
 if m == '_LIBCPP_ABI_VERSION':
 self.config.available_features.add('libcpp-abi-version-v%s'
Index: libcxx/trunk/docs/BuildingLibcxx.rst
===
--- libcxx/trunk/docs/BuildingLibcxx.rst
+++ libcxx/trunk/docs/BuildingLibcxx.rst
@@ -332,6 +332,15 @@
   Use the specified GCC toolchain and standard library when building the native
   stdlib benchmark tests.
 
+.. option:: LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT:BOOL
+
+  **Default**: ``OFF``
+
+  Pick the default for whether to constrain ABI-unstable symbols to
+  each individual translation unit. This setting controls whether
+  `_LIBCPP_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` is defined by default --
+  see the documentation of that macro for details.
+
 
 libc++ ABI Feature Options
 --
Index: libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
===
--- libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
+++ libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst
@@ -42,9 +42,7 @@
 
 **_LIBCPP_HIDE_FROM_ABI**
   Mark a function as not being part of the ABI of any final linked image that
-  uses it, and also as being internal to each TU that uses that function. In
-  other words, the address of a function marked with this attribute is not
-  guaranteed to be the same across translation units.
+  uses it.
 
 **_LIBCPP_HIDE_FROM_ABI_AFTER_V1**
   Mark a function as being hidden from the ABI (per `_LIBCPP_HIDE_FROM_ABI`)
@@ -61,6 +59,41 @@
   ABI, we should create a new _LIBCPP_HIDE_FROM_ABI_AFTER_XXX macro, and we can
   use it to start removing symbols from the ABI after that stable version.
 
+**_LIBCPP_HIDE_FROM_ABI_PER_TU**
+  This macro controls whether symbols hidden from the ABI with `_LIBCPP_HIDE_FROM_ABI`
+  are local to each translation unit in addition to being local to each final
+  linked image. This macro is defined to either 0 or 1. When it is defined to
+  1, translation units compiled with different versions of libc++ can be linked
+  together, since all non ABI-facing functions are local to each translation unit.
+  This allows static archives built with different versions of libc++ to be linked
+  together. This also means that functions marked with `_LIBCPP_HIDE_FROM_ABI`
+  are not guaranteed to have the same address across translation unit boundaries.
+
+  When the macro is defined to 0, there is no guarantee that translation units
+  compiled with di

[PATCH] D50876: Clean up newly created header

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with the `__clz` you missed.




Comment at: include/bit:61
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x)   { return __builtin_popcount  (__x); }
+

Funny spacing between `__builtin_popcount` and `(__x)`


https://reviews.llvm.org/D50876



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D50652#1202524, @hans wrote:

> Thanks! Merged to 7.0 in r339882.


Now that this has been done, I guess we need to document somewhere in the 
release notes that the default contract given by libc++ is changing in LLVM 7, 
right?


Repository:
  rL LLVM

https://reviews.llvm.org/D50652



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

[PATCH] D47814: Teach libc++ to use native NetBSD's max_align_t

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: include/stddef.h:55
 
 // Re-use the compiler's  max_align_t where possible.
 #if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \

chandlerc wrote:
> Unrelated to your patch, but this comment is now amazingly out of place.
Nope, based on `git blame` I think it's still where it should be.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47814



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

[PATCH] D51043: [clang][NFC] Fix typo in the name of a note

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: ahatanak.
Herald added subscribers: cfe-commits, dexonsmith.

r306722 introduced a new note called 
note_silence_unligned_allocation_unavailable
where I believe what was meant is note_silence_aligned_allocation_unavailable.


Repository:
  rCXX libc++

https://reviews.llvm.org/D51043

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExprCXX.cpp


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -1744,7 +1744,7 @@
 S.Diag(Loc, diag::err_aligned_allocation_unavailable)
 << IsDelete << FD.getType().getAsString() << OSName
 << alignedAllocMinVersion(T.getOS()).getAsString();
-S.Diag(Loc, diag::note_silence_unligned_allocation_unavailable);
+S.Diag(Loc, diag::note_silence_aligned_allocation_unavailable);
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6468,7 +6468,7 @@
 def err_aligned_allocation_unavailable : Error<
   "aligned %select{allocation|deallocation}0 function of type '%1' is only "
   "available on %2 %3 or newer">;
-def note_silence_unligned_allocation_unavailable : Note<
+def note_silence_aligned_allocation_unavailable : Note<
   "if you supply your own aligned allocation functions, use "
   "-faligned-allocation to silence this diagnostic">;
 


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -1744,7 +1744,7 @@
 S.Diag(Loc, diag::err_aligned_allocation_unavailable)
 << IsDelete << FD.getType().getAsString() << OSName
 << alignedAllocMinVersion(T.getOS()).getAsString();
-S.Diag(Loc, diag::note_silence_unligned_allocation_unavailable);
+S.Diag(Loc, diag::note_silence_aligned_allocation_unavailable);
   }
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6468,7 +6468,7 @@
 def err_aligned_allocation_unavailable : Error<
   "aligned %select{allocation|deallocation}0 function of type '%1' is only "
   "available on %2 %3 or newer">;
-def note_silence_unligned_allocation_unavailable : Note<
+def note_silence_aligned_allocation_unavailable : Note<
   "if you supply your own aligned allocation functions, use "
   "-faligned-allocation to silence this diagnostic">;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51043: [clang][NFC] Fix typo in the name of a note

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340288: [clang][NFC] Fix typo in the name of a note 
(authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51043?vs=161737&id=161739#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51043

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaExprCXX.cpp


Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6468,7 +6468,7 @@
 def err_aligned_allocation_unavailable : Error<
   "aligned %select{allocation|deallocation}0 function of type '%1' is only "
   "available on %2 %3 or newer">;
-def note_silence_unligned_allocation_unavailable : Note<
+def note_silence_aligned_allocation_unavailable : Note<
   "if you supply your own aligned allocation functions, use "
   "-faligned-allocation to silence this diagnostic">;
 
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -1744,7 +1744,7 @@
 S.Diag(Loc, diag::err_aligned_allocation_unavailable)
 << IsDelete << FD.getType().getAsString() << OSName
 << alignedAllocMinVersion(T.getOS()).getAsString();
-S.Diag(Loc, diag::note_silence_unligned_allocation_unavailable);
+S.Diag(Loc, diag::note_silence_aligned_allocation_unavailable);
   }
 }
 


Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6468,7 +6468,7 @@
 def err_aligned_allocation_unavailable : Error<
   "aligned %select{allocation|deallocation}0 function of type '%1' is only "
   "available on %2 %3 or newer">;
-def note_silence_unligned_allocation_unavailable : Note<
+def note_silence_aligned_allocation_unavailable : Note<
   "if you supply your own aligned allocation functions, use "
   "-faligned-allocation to silence this diagnostic">;
 
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -1744,7 +1744,7 @@
 S.Diag(Loc, diag::err_aligned_allocation_unavailable)
 << IsDelete << FD.getType().getAsString() << OSName
 << alignedAllocMinVersion(T.getOS()).getAsString();
-S.Diag(Loc, diag::note_silence_unligned_allocation_unavailable);
+S.Diag(Loc, diag::note_silence_aligned_allocation_unavailable);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51170: [libc++] Remove race condition in std::async

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: mclow.lists.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

The state associated to the future was set in one thread (with synchronization)
but read in another thread without synchronization, which led to a data race.

https://bugs.llvm.org/show_bug.cgi?id=38682
rdar://problem/42548261


Repository:
  rCXX libc++

https://reviews.llvm.org/D51170

Files:
  libcxx/include/future
  libcxx/src/future.cpp
  libcxx/test/std/thread/futures/futures.async/async_race.38682.pass.cpp

Index: libcxx/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
@@ -0,0 +1,58 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++98, c++03
+
+// This test is designed to cause and allow TSAN to detect a race condition
+// in std::async, as reported in https://bugs.llvm.org/show_bug.cgi?id=38682.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+static int worker(std::vector const& data) {
+  return std::accumulate(data.begin(), data.end(), 0);
+}
+
+static int& worker_ref(int& i) { return i; }
+
+static void worker_void() { }
+
+int main() {
+  // future
+  {
+std::vector const v{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker, v);
+  int answer = fut.get();
+  assert(answer == 55);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_ref, std::ref(i));
+  int& answer = fut.get();
+  assert(answer == i);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_void);
+  fut.get();
+}
+  }
+}
Index: libcxx/src/future.cpp
===
--- libcxx/src/future.cpp
+++ libcxx/src/future.cpp
@@ -179,10 +179,7 @@
 future::future(__assoc_sub_state* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 future::~future()
Index: libcxx/include/future
===
--- libcxx/include/future
+++ libcxx/include/future
@@ -556,13 +556,14 @@
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 
 _LIBCPP_INLINE_VISIBILITY
-void __set_future_attached()
-{
+void __attach_future() {
 lock_guard __lk(__mut_);
+bool __has_future_attached = (__state_ & __future_attached) != 0;
+if (__has_future_attached)
+__throw_future_error(future_errc::future_already_retrieved);
+this->__add_shared();
 __state_ |= __future_attached;
 }
-_LIBCPP_INLINE_VISIBILITY
-bool __has_future_attached() const {return (__state_ & __future_attached) != 0;}
 
 _LIBCPP_INLINE_VISIBILITY
 void __set_deferred() {__state_ |= deferred;}
@@ -1154,10 +1155,7 @@
 future<_Rp>::future(__assoc_state<_Rp>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 struct __release_shared_count
@@ -1257,10 +1255,7 @@
 future<_Rp&>::future(__assoc_state<_Rp&>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51172: [libcxx] Comment out #define __cpp_lib_node_extract, we only support half of that functionality

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM. Don't forget to update https://reviews.llvm.org/D48896 so it uncomments 
this. Also, this should be merged into LLVM 7.


Repository:
  rCXX libc++

https://reviews.llvm.org/D51172



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

[PATCH] D51170: [libc++] Remove race condition in std::async

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/include/future:556
 bool __has_value() const
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 

jfb wrote:
> I'm not auditing everything, but it seems like code above can still access 
> __state_ without holding __mut_? Like in the dtor.
> 
> Generally this patch lgtm because it's a step forward, but maybe we should 
> separately refactor the code to make it so that accesses to __state_ require 
> passing in a reference to lock_guard to show we actually hold __mut_. It 
> would ignore that reference, but that's a way to enforce, in the type system, 
> that __state_ is only touched when the lock is held.
> 
> WDYT?
I think you're right, and I filed this bug to keep track of the issue: 
https://bugs.llvm.org/show_bug.cgi?id=38688

Not all of them need a lock (some are in the constructor where only one thread 
has a reference to the data, for example), but most of them probably do.


Repository:
  rCXX libc++

https://reviews.llvm.org/D51170



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

[PATCH] D51170: [libc++] Remove race condition in std::async

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340608: [libc++] Remove race condition in std::async 
(authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51170?vs=162200&id=162376#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51170

Files:
  libcxx/trunk/include/future
  libcxx/trunk/src/future.cpp
  libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp

Index: libcxx/trunk/src/future.cpp
===
--- libcxx/trunk/src/future.cpp
+++ libcxx/trunk/src/future.cpp
@@ -179,10 +179,7 @@
 future::future(__assoc_sub_state* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 future::~future()
Index: libcxx/trunk/include/future
===
--- libcxx/trunk/include/future
+++ libcxx/trunk/include/future
@@ -556,13 +556,14 @@
 {return (__state_ & __constructed) || (__exception_ != nullptr);}
 
 _LIBCPP_INLINE_VISIBILITY
-void __set_future_attached()
-{
+void __attach_future() {
 lock_guard __lk(__mut_);
+bool __has_future_attached = (__state_ & __future_attached) != 0;
+if (__has_future_attached)
+__throw_future_error(future_errc::future_already_retrieved);
+this->__add_shared();
 __state_ |= __future_attached;
 }
-_LIBCPP_INLINE_VISIBILITY
-bool __has_future_attached() const {return (__state_ & __future_attached) != 0;}
 
 _LIBCPP_INLINE_VISIBILITY
 void __set_deferred() {__state_ |= deferred;}
@@ -1154,10 +1155,7 @@
 future<_Rp>::future(__assoc_state<_Rp>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 struct __release_shared_count
@@ -1257,10 +1255,7 @@
 future<_Rp&>::future(__assoc_state<_Rp&>* __state)
 : __state_(__state)
 {
-if (__state_->__has_future_attached())
-__throw_future_error(future_errc::future_already_retrieved);
-__state_->__add_shared();
-__state_->__set_future_attached();
+__state_->__attach_future();
 }
 
 template 
Index: libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
===
--- libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
+++ libcxx/trunk/test/std/thread/futures/futures.async/async_race.38682.pass.cpp
@@ -0,0 +1,58 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++98, c++03
+
+// This test is designed to cause and allow TSAN to detect a race condition
+// in std::async, as reported in https://bugs.llvm.org/show_bug.cgi?id=38682.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+static int worker(std::vector const& data) {
+  return std::accumulate(data.begin(), data.end(), 0);
+}
+
+static int& worker_ref(int& i) { return i; }
+
+static void worker_void() { }
+
+int main() {
+  // future
+  {
+std::vector const v{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker, v);
+  int answer = fut.get();
+  assert(answer == 55);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_ref, std::ref(i));
+  int& answer = fut.get();
+  assert(answer == i);
+}
+  }
+
+  // future
+  {
+for (int i = 0; i != 20; ++i) {
+  std::future fut = std::async(std::launch::async, worker_void);
+  fut.get();
+}
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50534: [libc++] Fix handling of negated character classes in regex

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX340609: [libc++] Fix handling of negated character classes 
in regex (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50534?vs=160169&id=162377#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D50534

Files:
  include/regex
  test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
  test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp


Index: include/regex
===
--- include/regex
+++ include/regex
@@ -2414,20 +2414,17 @@
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.
+if (!(__neg_mask_ == 0 && __neg_chars_.empty()))
 {
-  const bool __in_neg_mask = (__neg_mask_ == 0) ||
-  __traits_.isctype(__ch, __neg_mask_);
+const bool __in_neg_mask = __traits_.isctype(__ch, __neg_mask_);
   const bool __in_neg_chars =
-  __neg_chars_.empty() ||
   std::find(__neg_chars_.begin(), __neg_chars_.end(), __ch) !=
   __neg_chars_.end();
   if (!(__in_neg_mask || __in_neg_chars))
Index: test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
===
--- test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
+++ test/std/re/re.alg/re.alg.search/invert_neg_word_search.pass.cpp
@@ -18,7 +18,7 @@
 
 #include 
 #include 
-#include "test_macros.h"
+
 
 // PR34310
 int main()
Index: test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
===
--- test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
+++ test/std/re/re.alg/re.alg.match/inverted_character_classes.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+// UNSUPPORTED: c++98, c++03
+
+// Make sure that we correctly match inverted character classes.
+
+#include 
+#include 
+
+
+int main() {
+assert(std::regex_match("X", std::regex("[X]")));
+assert(std::regex_match("X", std::regex("[XY]")));
+assert(!std::regex_match("X", std::regex("[^X]")));
+assert(!std::regex_match("X", std::regex("[^XY]")));
+
+assert(std::regex_match("X", std::regex("[\\S]")));
+assert(!std::regex_match("X", std::regex("[^\\S]")));
+
+assert(!std::regex_match("X", std::regex("[\\s]")));
+assert(std::regex_match("X", std::regex("[^\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\s\\S]")));
+assert(std::regex_match("X", std::regex("[^Y\\s]")));
+assert(!std::regex_match("X", std::regex("[^X\\s]")));
+
+assert(std::regex_match("X", std::regex("[\\w]")));
+assert(std::regex_match("_", std::regex("[\\w]")));
+assert(!std::regex_match("X", std::regex("[^\\w]")));
+assert(!std::regex_match("_", std::regex("[^\\w]")));
+
+assert(!std::regex_match("X", std::regex("[\\W]")));
+assert(!std::regex_match("_", std::regex("[\\W]")));
+assert(std::regex_match("X", std::regex("[^\\W]")));
+assert(std::regex_match("_", std::regex("[^\\W]")));
+}


Index: include/regex
===
--- include/regex
+++ include/regex
@@ -2414,20 +2414,17 @@
 goto __exit;
 }
 }
-// set of "__found" chars =
+// When there's at least one of __neg_chars_ and __neg_mask_, the set
+// of "__found" chars is
 //   union(complement(union(__neg_chars_, __neg_mask_)),
 // other cases...)
 //
-// __neg_chars_ and __neg_mask_'d better be handled together, as there
-// are no short circuit opportunities.
-//
-// In addition, when __neg_mask_/__neg_chars_ is empty, they should be
-// treated as all ones/all chars.
+// It doesn't make sense to check this when there are no __neg_chars_
+// and no __neg_mask_.

[PATCH] D51955: Create infrastructure for defining and testing feature test macros

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Thanks! I don't like feature test macros either, but we should still strive for 
conformance.


https://reviews.llvm.org/D51955



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

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: rsmith.
Herald added subscribers: cfe-commits, dexonsmith.

Attributes on member classes of class templates (and other similar entities)
are not currently instantiated. This was discovered by Richard Smith here:

  http://lists.llvm.org/pipermail/cfe-dev/2018-September/059291.html

This commit makes sure that attributes are instantiated properly.

PR38913


Repository:
  rC Clang

https://reviews.llvm.org/D51997

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/PR38913.cpp


Index: clang/test/SemaCXX/PR38913.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38913.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | 
FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct 
__attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv
+
+// ...a member partial specialization
+template struct C {
+  template struct X { };
+  template struct __attribute__((abi_tag("CTAG"))) X { };
+};
+C::X* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+
+// ...a member explicit specialization
+template struct D {
+  template struct X { };
+  template<> struct __attribute__((abi_tag("DTAG"))) X { };
+};
+D::X* d() { return 0; } // CHECK-DAG: @_Z1dB4DTAGv
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1264,6 +1264,10 @@
   assert(!(isFriend && Owner->isDependentContext()));
   Inst->setPreviousDecl(PrevClassTemplate);
 
+  // Instantiate the attributes on both the template declaration and the 
associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, 
StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, 
LateAttrs, StartingScope);
+
   RecordInst->setDescribedClassTemplate(Inst);
 
   if (isFriend) {
@@ -1491,6 +1495,8 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs, 
StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access


Index: clang/test/SemaCXX/PR38913.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38913.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct __attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv
+
+// ...a member partial specialization
+template struct C {
+  template struct X { };
+  template struct __attribute__((abi_tag("CTAG"))) X { };
+};
+C::X* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+
+// ...a member explicit specialization
+template struct D {
+  template struct X { };
+  template<> struct __attribute__((abi_tag("DTAG"))) X { };
+};
+D::X* d() { return 0; } // CHECK-DAG: @_Z1dB4DTAGv
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1264,6 +1264,10 @@
   assert(!(isFriend && Owner->isDependentContext()));
   Inst->setPreviousDecl(PrevClassTemplate);
 
+  // Instantiate the attributes on both the template declaration and the associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs, StartingScope);
+
   RecordInst->setDescribedClassTemplate(Inst);
 
   if (isFriend) {
@@ -1491,6 +1495,8 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs, StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access
___
cfe-commits mailing list
cfe

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

This patch is missing support for partial specializations and explicit 
specializations. The part I don't understand is how to get the `CXXRecordDecl`s 
to have the right attribute below. Here's the AST dump for the test file with 
the current patch:

  TranslationUnitDecl 0x7f8a2a00a8e8 <> 
  |-TypedefDecl 0x7f8a2a00b1c0 <>  implicit 
__int128_t '__int128'
  | `-BuiltinType 0x7f8a2a00ae80 '__int128'
  |-TypedefDecl 0x7f8a2a00b228 <>  implicit 
__uint128_t 'unsigned __int128'
  | `-BuiltinType 0x7f8a2a00aea0 'unsigned __int128'
  |-TypedefDecl 0x7f8a2a00b558 <>  implicit 
__NSConstantString '__NSConstantString_tag'
  | `-RecordType 0x7f8a2a00b300 '__NSConstantString_tag'
  |   `-CXXRecord 0x7f8a2a00b278 '__NSConstantString_tag'
  |-TypedefDecl 0x7f8a28827600 <>  implicit 
__builtin_ms_va_list 'char *'
  | `-PointerType 0x7f8a2a00b5b0 'char *'
  |   `-BuiltinType 0x7f8a2a00a980 'char'
  |-TypedefDecl 0x7f8a28827928 <>  implicit 
__builtin_va_list '__va_list_tag [1]'
  | `-ConstantArrayType 0x7f8a288278d0 '__va_list_tag [1]' 1
  |   `-RecordType 0x7f8a288276e0 '__va_list_tag'
  | `-CXXRecord 0x7f8a28827650 '__va_list_tag'
  |-ClassTemplateDecl 0x7f8a28827ab8 
 col:26 A
  | |-TemplateTypeParmDecl 0x7f8a28827978  col:16 class depth 0 
index 0 T
  | |-CXXRecordDecl 0x7f8a28827a30  col:26 struct A definition
  | | |-DefinitionData empty aggregate standard_layout trivially_copyable pod 
trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | | | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveAssignment exists simple trivial needs_implicit
  | | | `-Destructor simple irrelevant trivial needs_implicit
  | | |-CXXRecordDecl 0x7f8a28827d00  col:26 implicit struct A
  | | `-CXXRecordDecl 0x7f8a28827e38  col:70 struct X definition
  | |   |-DefinitionData empty aggregate standard_layout trivially_copyable pod 
trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  | |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | |   | |-MoveConstructor exists simple trivial needs_implicit
  | |   | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | |   | |-MoveAssignment exists simple trivial needs_implicit
  | |   | `-Destructor simple irrelevant trivial needs_implicit
  | |   |-AbiTagAttr 0x7f8a28827f48  ATAG
  | |   `-CXXRecordDecl 0x7f8a28827fa8  col:70 implicit struct X
  | `-ClassTemplateSpecializationDecl 0x7f8a28828088  col:26 
struct A definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   |-TemplateArgument type 'int'
  |   |-CXXRecordDecl 0x7f8a28828280 prev 0x7f8a28828088  
col:26 implicit struct A
  |   `-CXXRecordDecl 0x7f8a28828308  col:70 referenced struct X
  | `-AbiTagAttr 0x7f8a288283b0  ATAG
  |-FunctionDecl 0x7f8a2885a200  col:12 a 'A::X *()'
  | `-CompoundStmt 0x7f8a2885a328 
  |   `-ReturnStmt 0x7f8a2885a310 
  | `-ImplicitCastExpr 0x7f8a2885a2f8  'A::X *' 
  |   `-IntegerLiteral 0x7f8a2885a2d8  'int' 0
  |-ClassTemplateDecl 0x7f8a2885a448  col:26 B
  | |-TemplateTypeParmDecl 0x7f8a2885a340  col:16 class depth 0 
index 0 T
  | |-CXXRecordDecl 0x7f8a2885a3c0  col:26 struct B definition
  | | |-DefinitionData empty aggregate standard_layout trivially_copyable pod 
trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | | | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveAssignment exists simple trivial needs_implicit
  | | | `-Destructor simple irrelevant trivial needs_implicit
  | | |-CXXRecordDecl 0x7f8a2885a690  col:26 implicit struct B
  | | `-ClassTemplateDecl 0x7f8a2885a888  col:88 X
  | |   |-TemplateTypeParmDecl 0x7f8a2885a718  col:45 class 
depth

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne marked 2 inline comments as done.
ldionne added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1268
+  // Instantiate the attributes on both the template declaration and the 
associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, 
StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, 
LateAttrs, StartingScope);

erik.pilkington wrote:
> Why are you instantiating the attributes onto the ClassTemplateDecl? At the 
> very least it seems wrong to instantiate attributes from the pattern to the 
> ClassTemplateDecl.
My understanding was that
- `Inst` represented `B::X` (the template)
- `RecordInst` represented `B::X` (the template specialization, which 
is a "real struct")
- `Pattern`  represented `B::X`

If that is right, then it seemed to make sense that both `B::X` and 
`B::X` should have the attribute, since `B::X` has it. I might 
be misunderstanding something though.



Comment at: clang/test/SemaCXX/PR38913.cpp:19
+};
+C::X* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+

erik.pilkington wrote:
> ldionne wrote:
> > This test is failing in the current iteration of the patch.
> I think the problem here is that we don't instantiate X because 
> it's behind the pointer, so we're just considering the primary template. 
> Looks like we do add the attribute (with the fix above) here:
> ```
> template struct C {
>   template struct X { };
>   template struct __attribute__((abi_tag("CTAG"))) X { };
> };
> C::X c() { return 0; }
> ```
> But we don't get the right mangling, for some reason. Note that this problem 
> is present even in the non-member case. 
> 
> I don't think attributes on specializations have particularly good support 
> anyways, so I wouldn't really lose any sleep if we "left this to a 
> follow-up". Maybe @rsmith has some thoughts here.
I'll drop support for specializations for now. The current patch is enough to 
unblock me on the `no_extern_template` attribute, which is the goal of all of 
this.


Repository:
  rC Clang

https://reviews.llvm.org/D51997



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

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1268
+  // Instantiate the attributes on both the template declaration and the 
associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, 
StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, 
LateAttrs, StartingScope);

rsmith wrote:
> ldionne wrote:
> > erik.pilkington wrote:
> > > Why are you instantiating the attributes onto the ClassTemplateDecl? At 
> > > the very least it seems wrong to instantiate attributes from the pattern 
> > > to the ClassTemplateDecl.
> > My understanding was that
> > - `Inst` represented `B::X` (the template)
> > - `RecordInst` represented `B::X` (the template specialization, 
> > which is a "real struct")
> > - `Pattern`  represented `B::X`
> > 
> > If that is right, then it seemed to make sense that both `B::X` and 
> > `B::X` should have the attribute, since `B::X` has it. I 
> > might be misunderstanding something though.
> That's almost right.
> 
>   * `Inst` represents `B::X` (the template declaration in the 
> instantiation of `B`).
>   * `RecordInst` represents `B::X` (the class declaration within the 
> template declaration in the instantiation of `B`).
>   * `Pattern` represents `B::X` (the class declaration within the 
> template declaration in the definition of `B`).
> 
> And we don't yet have a value for `U`, so there is no `B::X` here.
> 
> That is:
> 
> ```
> template
>   struct B {
> template // #1
>   struct X; // Pattern
>   };
> // implicit instantiation of B:
> template<> struct B {
>   template // Inst
> struct X; // RecordInst
> };
> ```
> 
> A template-declaration can never have attributes on it (there is no syntactic 
> location where they can be written), so we should never be instantiating 
> attributes onto it. If it could, we should instantiate those attributes from 
> `#1`, not from `Pattern`.
Thanks a lot for the explanation -- this is really helpful. It's now clear that 
this line should not be there.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2971
   InstD->setExternLoc(D->getExternLoc());
   InstD->setTemplateKeywordLoc(D->getTemplateKeywordLoc());
 

ldionne wrote:
> I tried a couple variations of things like:
> 
> ```
> SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, InstD, LateAttrs, 
> StartingScope);
> ```
> 
> and 
> 
> ```
> SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, 
> ClassTemplate->getTemplatedDecl(), LateAttrs, StartingScope);
> ```
> 
> For some reason, none of them end up instantiating the attribute on the 
> `CXXRecordDecl` that is used to determine the mangling.
> 
So, assuming this:

```
template struct Foo {
  template struct X { };
  template<> struct ATTRS X { };
};
```

Based on what @rsmith says about partial specializations, my understanding is 
that:

- `D` is the explicit specialization inside the general class template: 
`Foo::X`
- `InstD` is the explicit specialization inside the specialization of 
`Foo`: `Foo::X`

And hence, what we want to do here is apply the attributes from 
`Foo::X` onto `Foo::X`:

```
SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, InstD, LateAttrs, 
StartingScope);
```

However, like I said previously this is not sufficient and I'll just not 
implement this for specializations for the time being (for the sake of making 
progress on `no_extern_template`).


Repository:
  rC Clang

https://reviews.llvm.org/D51997



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

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 165375.
ldionne added a comment.

Drop support for partial specializations and explicit specializations.
Address comments by Erik.


Repository:
  rC Clang

https://reviews.llvm.org/D51997

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/PR38913.cpp


Index: clang/test/SemaCXX/PR38913.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38913.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | 
FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct 
__attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1258,6 +1258,9 @@
   if (QualifierLoc)
 RecordInst->setQualifierInfo(QualifierLoc);
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs,
+  StartingScope);
+
   ClassTemplateDecl *Inst
 = ClassTemplateDecl::Create(SemaRef.Context, DC, D->getLocation(),
 D->getIdentifier(), InstParams, RecordInst);
@@ -1491,6 +1494,9 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs,
+  StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access


Index: clang/test/SemaCXX/PR38913.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38913.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct __attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1258,6 +1258,9 @@
   if (QualifierLoc)
 RecordInst->setQualifierInfo(QualifierLoc);
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs,
+  StartingScope);
+
   ClassTemplateDecl *Inst
 = ClassTemplateDecl::Create(SemaRef.Context, DC, D->getLocation(),
 D->getIdentifier(), InstParams, RecordInst);
@@ -1491,6 +1494,9 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs,
+  StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I created https://bugs.llvm.org/show_bug.cgi?id=38942 to keep track of the 
problem for partial specializations and explicit specializations. Moving 
forward with this patch will allow me to land the `no_extern_template` 
attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D51997



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

[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342238: [clang] Make sure attributes on member classes are 
applied properly (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51997?vs=165375&id=165502#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51997

Files:
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/SemaCXX/PR38913.cpp


Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1258,6 +1258,9 @@
   if (QualifierLoc)
 RecordInst->setQualifierInfo(QualifierLoc);
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs,
+  StartingScope);
+
   ClassTemplateDecl *Inst
 = ClassTemplateDecl::Create(SemaRef.Context, DC, D->getLocation(),
 D->getIdentifier(), InstParams, RecordInst);
@@ -1491,6 +1494,9 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs,
+  StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access
Index: test/SemaCXX/PR38913.cpp
===
--- test/SemaCXX/PR38913.cpp
+++ test/SemaCXX/PR38913.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | 
FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct 
__attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv


Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1258,6 +1258,9 @@
   if (QualifierLoc)
 RecordInst->setQualifierInfo(QualifierLoc);
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs,
+  StartingScope);
+
   ClassTemplateDecl *Inst
 = ClassTemplateDecl::Create(SemaRef.Context, DC, D->getLocation(),
 D->getIdentifier(), InstParams, RecordInst);
@@ -1491,6 +1494,9 @@
   if (SubstQualifier(D, Record))
 return nullptr;
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs,
+  StartingScope);
+
   Record->setImplicit(D->isImplicit());
   // FIXME: Check against AS_none is an ugly hack to work around the issue that
   // the tag decls introduced by friend class declarations don't have an access
Index: test/SemaCXX/PR38913.cpp
===
--- test/SemaCXX/PR38913.cpp
+++ test/SemaCXX/PR38913.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// PR38913
+// Check that we instantiate attributes on declarations for...
+
+// ...a member class of a class template specialization
+template struct A { struct __attribute__((abi_tag("ATAG"))) X { }; };
+A::X* a() { return 0; } // CHECK-DAG: @_Z1aB4ATAGv
+
+// ...a member class template
+template struct B { template struct __attribute__((abi_tag("BTAG"))) X { }; };
+B::X* b() { return 0; } // CHECK-DAG: @_Z1bB4BTAGv
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51789: [WIP][clang] Add the no_extern_template attribute

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 165507.
ldionne added a comment.

Fix the tests and remove some warnings that I wasn't able to generate properly
(to avoid false positives).


Repository:
  rC Clang

https://reviews.llvm.org/D51789

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  
clang/test/CodeGenCXX/attr-no_extern_template.dont_assume_extern_instantiation.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-no_extern_template.diagnose_on_undefined_entity.cpp
  clang/test/SemaCXX/attr-no_extern_template.explicit_instantiation.cpp
  clang/test/SemaCXX/attr-no_extern_template.extern_declaration.cpp
  clang/test/SemaCXX/attr-no_extern_template.merge_redeclarations.cpp

Index: clang/test/SemaCXX/attr-no_extern_template.merge_redeclarations.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-no_extern_template.merge_redeclarations.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Test that we properly merge the no_extern_template attribute on
+// redeclarations.
+
+#define NO_EXTERN_TEMPLATE __attribute__((no_extern_template))
+
+template 
+struct Foo {
+  // Declaration without the attribute, definition with the attribute.
+  void func1();
+
+  // Declaration with the attribute, definition without the attribute.
+  NO_EXTERN_TEMPLATE void func2();
+
+  // Declaration with the attribute, definition with the attribute.
+  NO_EXTERN_TEMPLATE void func3();
+};
+
+template 
+NO_EXTERN_TEMPLATE void Foo::func1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::func2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+NO_EXTERN_TEMPLATE void Foo::func3() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+struct Empty { };
+extern template struct Foo;
+
+int main() {
+  Foo foo;
+  foo.func1(); // expected-note{{in instantiation of}}
+  foo.func2(); // expected-note{{in instantiation of}}
+  foo.func3(); // expected-note{{in instantiation of}}
+}
Index: clang/test/SemaCXX/attr-no_extern_template.extern_declaration.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-no_extern_template.extern_declaration.cpp
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -Wno-unused-local-typedef -fsyntax-only -verify %s
+
+// Test that extern instantiation declarations cause members marked with
+// no_extern_template to be instantiated in the current TU.
+
+#define NO_EXTERN_TEMPLATE __attribute__((no_extern_template))
+
+template 
+struct Foo {
+  NO_EXTERN_TEMPLATE inline void non_static_member_function1();
+
+  NO_EXTERN_TEMPLATE void non_static_member_function2();
+
+  NO_EXTERN_TEMPLATE static inline void static_member_function1();
+
+  NO_EXTERN_TEMPLATE static void static_member_function2();
+
+  NO_EXTERN_TEMPLATE static int static_data_member;
+
+  struct NO_EXTERN_TEMPLATE member_class1 {
+static void static_member_function() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+  };
+
+  struct member_class2 {
+NO_EXTERN_TEMPLATE static void static_member_function() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+  };
+};
+
+template 
+inline void Foo::non_static_member_function1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::non_static_member_function2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+inline void Foo::static_member_function1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::static_member_function2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+int Foo::static_data_member = T::invalid; // expected-error{{no member named 'invalid' in 'Empty'}}
+
+struct Empty { };
+extern template struct Foo;
+
+int main() {
+  Foo foo;
+  foo.non_static_member_function1();   // expected-note{{in instantiation of}}
+  foo.non_static_member_function2();   // expected-note{{in instantiation of}}
+  Foo::static_member_function1();   // expected-note{{in instantiation of}}
+  Foo::static_member_function2();   // expected-note{{in instantiation of}}
+  (void)foo.static_data_member;// expected-note{{in instantiation of}}
+  Foo::member_class1::static_member_function(); // expected-note{{in instantiation of}}
+  Foo::member_class2::

[PATCH] D51789: [clang] Add the no_extern_template attribute

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I think now's a good time to bikeshed the name of the attribute if you have 
other suggestions.


Repository:
  rC Clang

https://reviews.llvm.org/D51789



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

[PATCH] D51789: [clang] Add the no_extern_template attribute

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D51789#1235096, @rsmith wrote:

> In https://reviews.llvm.org/D51789#1234903, @ldionne wrote:
>
> > I think now's a good time to bikeshed the name of the attribute if you have 
> > other suggestions.
>
>
> OK, so the semantics of this attribute are "explicit instantiation 
> declarations or definitions applied to the enclosing class do not apply to 
> this member", right? So it opts the member out of both `extern template` and 
> (non-`extern`) `template`, but only when applied to an enclosing class.


Yes, but it also (obviously) opts the member out when applied to the member 
itself, not only to an enclosing class.

> Maybe something like `exclude_from_explicit_instantiation`? That's a little 
> longer than I'd like, but if it's going to be hidden behind a macro most of 
> the time I could live with it.

I like this a lot too. I thought about that one and was deterred by the length, 
but it may be acceptable. Realistically, it's also not an attribute we want 
people to use most of the time.

> Or maybe `no_transitive_instantiation`? That's less explicit about the 
> purpose of the attribute, but is a more comfortable length.

I'm not sure this one describes what the attribute is doing sufficiently 
closely.

Unless we get a better suggestion soonish, I'll change it to 
`exclude_from_explicit_instantiation` and update the diff.


Repository:
  rC Clang

https://reviews.llvm.org/D51789



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

[PATCH] D51789: [clang] Add the no_extern_template attribute

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D51789#1235113, @rsmith wrote:

> In https://reviews.llvm.org/D51789#1235100, @ldionne wrote:
>
> > In https://reviews.llvm.org/D51789#1235096, @rsmith wrote:
> >
> > > OK, so the semantics of this attribute are "explicit instantiation 
> > > declarations or definitions applied to the enclosing class do not apply 
> > > to this member", right? So it opts the member out of both `extern 
> > > template` and (non-`extern`) `template`, but only when applied to an 
> > > enclosing class.
> >
> >
> > Yes, but it also (obviously) opts the member out when applied to the member 
> > itself, not only to an enclosing class.
>
>
> Assuming I'm understanding you correctly, I don't think the current 
> implementation does that (nor does the documentation change say that), and I 
> think that's a feature not a bug. For example, given:
>
>   template struct A {
> [[clang::no_extern_template]] void f() {}
> void g() {}
>   };
>   template struct A; // instantiates A and definition of 
> A::g(), does not instantiate definition of A::f()
>   template void A::f(); // instantiates definition of A::f() 
> despite attribute
>
>
> ... the attribute affects the first explicit instantiation but not the second 
> (I think you're saying it should affect both, and make the second explicit 
> instantiation have no effect). I think the current behavior in this patch is 
> appropriate: making the attribute also affect the second case makes it 
> strictly less useful. [...]


Sorry, that's a misunderstanding. We're on the same page. What I meant is that 
if you apply the attribute to the entity itself but explicitly instantiate the 
enclosing class, then the entity is not instantiated. It's very obvious, but 
your previous comment:

> So it opts the member out of both `extern template` and (non-`extern`) 
> `template`, but only when applied to an enclosing class.

suggested that it only applied in the following case:

  template  struct Foo {
struct __attribute__((no_extern_template)) nested { static void f() { } };
  };
  
  template struct Foo; // Foo::nested::f not instantiated


Repository:
  rC Clang

https://reviews.llvm.org/D51789



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

[PATCH] D51789: [clang] Add the no_extern_template attribute

[Louis Dionne via Phabricator via cfe-commits]

ldionne updated this revision to Diff 165569.
ldionne added a comment.

Change no_extern_template to exclude_from_explicit_instantiation


Repository:
  rC Clang

https://reviews.llvm.org/D51789

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  
clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.dont_assume_extern_instantiation.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  
clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.diagnose_on_undefined_entity.cpp
  
clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.explicit_instantiation.cpp
  
clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.extern_declaration.cpp
  
clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.merge_redeclarations.cpp

Index: clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.merge_redeclarations.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.merge_redeclarations.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Test that we properly merge the exclude_from_explicit_instantiation
+// attribute on redeclarations.
+
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template 
+struct Foo {
+  // Declaration without the attribute, definition with the attribute.
+  void func1();
+
+  // Declaration with the attribute, definition without the attribute.
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION void func2();
+
+  // Declaration with the attribute, definition with the attribute.
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION void func3();
+};
+
+template 
+EXCLUDE_FROM_EXPLICIT_INSTANTIATION void Foo::func1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::func2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+EXCLUDE_FROM_EXPLICIT_INSTANTIATION void Foo::func3() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+struct Empty { };
+extern template struct Foo;
+
+int main() {
+  Foo foo;
+  foo.func1(); // expected-note{{in instantiation of}}
+  foo.func2(); // expected-note{{in instantiation of}}
+  foo.func3(); // expected-note{{in instantiation of}}
+}
Index: clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.extern_declaration.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.extern_declaration.cpp
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -Wno-unused-local-typedef -fsyntax-only -verify %s
+
+// Test that extern instantiation declarations cause members marked with
+// the exclude_from_explicit_instantiation attribute to be instantiated in
+// the current TU.
+
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template 
+struct Foo {
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION inline void non_static_member_function1();
+
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION void non_static_member_function2();
+
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION static inline void static_member_function1();
+
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION static void static_member_function2();
+
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION static int static_data_member;
+
+  struct EXCLUDE_FROM_EXPLICIT_INSTANTIATION member_class1 {
+static void static_member_function() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+  };
+
+  struct member_class2 {
+EXCLUDE_FROM_EXPLICIT_INSTANTIATION static void static_member_function() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+  };
+};
+
+template 
+inline void Foo::non_static_member_function1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::non_static_member_function2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+inline void Foo::static_member_function1() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+void Foo::static_member_function2() {
+  using Fail = typename T::invalid; // expected-error{{no type named 'invalid' in 'Empty'}}
+}
+
+template 
+int Foo::static_data_member = T::invalid; // expected-error{{no member named 'invalid' in 'Empty'}}
+
+struct Empty { };
+extern template struct Foo;
+
+int main() {
+  Foo foo;
+  foo.non_static_member_function1();   // expected-note{{in instantiation of}}
+  foo.non_static_member_function2();   // e

[PATCH] D49502: [CMake] Support statically linking dependencies only to shared or static library

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/lib/CMakeLists.txt:269
+AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
+#if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
+#(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND 
HAVE_LIBCXXABI))

phosek wrote:
> ldionne wrote:
> > I don't understand why any of this needs to change -- can you please 
> > explain? Also, you probably didn't mean to leave the commented-out lines.
> The reason this change is needed the case when we're linking shared libc++abi 
> into shared libc++ in which case `${LIBCXX_CXX_ABI_LIBRARY}` will be set to 
> `cxxabi_shared` in `HandleLibCXXABI.cmake` but we cannot merge `libc++abi.so` 
> into `libc++.a`, so instead we force the use of `cxxabi_static` here.
> 
> Alternatively, we could modify `HandleLibCXXABI.cmake` to set two 
> dependencies, one for the static case and one for the shared case and use the 
> former one here.
> 
> Removed the commented out lines.
Thanks. There's something I still don't understand. If you are linking the ABI 
library dynamically, why would you want to merge it (well, the static version 
of it) into `libc++.a`? It seems like this is somewhat defeating the purpose of 
dynamically linking against the ABI library, no?



Comment at: libcxxabi/src/CMakeLists.txt:64
   # FIXME: Is it correct to prefer the static version of libunwind?
   if (NOT LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_shared OR 
HAVE_LIBUNWIND))
 list(APPEND LIBCXXABI_LIBRARIES unwind_shared)

Does this not need to change anymore? I think we'd have to set different flags 
for `cxxabi_shared` and `cxxabi_static`.



Repository:
  rCXX libc++

https://reviews.llvm.org/D49502



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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne edited subscribers, added: cfe-commits; removed: llvm-commits.
ldionne added a comment.

In https://reviews.llvm.org/D49509#1167500, @smeenai wrote:

> This went out to llvm-commits. You may wanna re-upload with cfe-commits added 
> instead.


Ah! That uncovers a deeper problem -- the instructions when setting up the 
monorepo with Arcanist say to copy `.arcconfig` from `llvm/`. This is wrong, as 
I guess it depends what component you will be submitting to.


Repository:
  rL LLVM

https://reviews.llvm.org/D49509



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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49509#1168084, @ldionne wrote:

> In https://reviews.llvm.org/D49509#1167500, @smeenai wrote:
>
> > This went out to llvm-commits. You may wanna re-upload with cfe-commits 
> > added instead.
>
>
> Ah! That uncovers a deeper problem -- the instructions when setting up the 
> monorepo with Arcanist say to copy `.arcconfig` from `llvm/`. This is wrong, 
> as I guess it depends what component you will be submitting to.


Nope, actually, that's not it. It's this Herald rule kicking in: F6725290: 
Screen Shot 2018-07-19 at 10.24.22.png 


Repository:
  rL LLVM

https://reviews.llvm.org/D49509



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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337477: [libc++] Allow running ABI list tests with different 
ABI versions (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49509?vs=156148&id=156323#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49509

Files:
  libcxx/trunk/lib/abi/CMakeLists.txt
  libcxx/trunk/lib/abi/x86_64-apple-darwin.abilist
  libcxx/trunk/lib/abi/x86_64-apple-darwin.v1.abilist
  libcxx/trunk/lib/abi/x86_64-apple-darwin.v2.abilist
  libcxx/trunk/lib/abi/x86_64-unknown-linux-gnu.abilist
  libcxx/trunk/lib/abi/x86_64-unknown-linux-gnu.v1.abilist



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

[PATCH] D49502: [CMake] Support statically linking dependencies only to shared or static library

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/lib/CMakeLists.txt:269
+AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
+#if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
+#(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND 
HAVE_LIBCXXABI))

phosek wrote:
> ldionne wrote:
> > phosek wrote:
> > > ldionne wrote:
> > > > I don't understand why any of this needs to change -- can you please 
> > > > explain? Also, you probably didn't mean to leave the commented-out 
> > > > lines.
> > > The reason this change is needed the case when we're linking shared 
> > > libc++abi into shared libc++ in which case `${LIBCXX_CXX_ABI_LIBRARY}` 
> > > will be set to `cxxabi_shared` in `HandleLibCXXABI.cmake` but we cannot 
> > > merge `libc++abi.so` into `libc++.a`, so instead we force the use of 
> > > `cxxabi_static` here.
> > > 
> > > Alternatively, we could modify `HandleLibCXXABI.cmake` to set two 
> > > dependencies, one for the static case and one for the shared case and use 
> > > the former one here.
> > > 
> > > Removed the commented out lines.
> > Thanks. There's something I still don't understand. If you are linking the 
> > ABI library dynamically, why would you want to merge it (well, the static 
> > version of it) into `libc++.a`? It seems like this is somewhat defeating 
> > the purpose of dynamically linking against the ABI library, no?
> In our case, we want to link shared library dynamically and merge all static 
> libraries, so `LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY=OFF` and 
> `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON`. This means that 
> `LIBCXX_CXX_ABI_LIBRARY` will be set to `cxxabi_shared` and it's what will be 
> used other places throughout this file as the interface library. However, we 
> cannot merge `libc++abi.so` into `libc++.a` so that's why we have to 
> explicitly select `cxxabi_static` here rather than simply using 
> `LIBCXX_CXX_ABI_LIBRARY` as before.
> 
> Like I mentioned in the previous comment, alternative would be to split this 
> into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and 
> `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?
Yes, I understand why the `TARGET_LINKER_FILE` must also be `cxxabi_static`. 
What I don't understand is why the conditions need to change, i.e. why you're 
going from
```
if (TARGET ${LIBCXX_CXX_ABI_LIBRARY} OR
(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND 
HAVE_LIBCXXABI))
```

to 

```
if (${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?"
AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
```

Note that I do think your new condition make more sense (since if the ABI 
library is not a target, we should fall back on the `else()` branch). I just 
want to understand why you're changing it.

> Like I mentioned in the previous comment, alternative would be to split this 
> into two variables, e.g. `LIBCXX_CXX_SHARED_ABI_LIBRARY` and 
> `LIBCXX_CXX_STATIC_ABI_LIBRARY`, would you prefer that approach?

I don't think that is necessary at this point.




Comment at: libcxxabi/src/CMakeLists.txt:64
   # FIXME: Is it correct to prefer the static version of libunwind?
   if (NOT LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_shared OR 
HAVE_LIBUNWIND))
 list(APPEND LIBCXXABI_LIBRARIES unwind_shared)

phosek wrote:
> ldionne wrote:
> > Does this not need to change anymore? I think we'd have to set different 
> > flags for `cxxabi_shared` and `cxxabi_static`.
> > 
> This is relying on the fact that `LIBCXXABI_LIBRARIES` isn't used for library 
> merging that's done on lines 158-162 and CMake should ignore shared library 
> dependencies passed to `target_link_libraries` when building static library 
> as done on line 163 (since linking dynamic library target against static 
> library doesn't make sense). This seems to be working fine in my testing, but 
> I'm also OK splitting this into two variables if you want it to be explicit.
I would rather see it handled explicitly.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49502



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

[PATCH] D49502: [CMake] Support statically linking dependencies only to shared or static library

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D49502



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

[PATCH] D49573: [CMake] Option to control whether shared/static library is installed

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I don't like the fact that we're adding options like crazy, thus making the 
build more complicated. If you don't want to have some libraries that were 
built, why not just remove them afterwards?


Repository:
  rL LLVM

https://reviews.llvm.org/D49573



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

[PATCH] D49584: [CMake] Install C++ ABI headers into the right location

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

This change LGTM. However, I do have a question about the overall change 
(including r335809 and r337118). Why is it that we're not simply relying on the 
`CMAKE_INSTALL_PREFIX` being set to the path where we should install instead of 
using all these custom variables  like `LIBCXX_INSTALL_HEADER_PREFIX`? I'm 
curious to understand what's special about your use case, since it does seem 
like we're doing things differently in a few places for Fuchsia.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49584



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

[PATCH] D49573: [CMake] Option to control whether shared/static library is installed

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Can't you simply set `LIBCXX_ENABLE_SHARED=OFF` when you don't want to 
build/install the shared library and `LIBCXX_ENABLE_STATIC=OFF` for the static 
library?


Repository:
  rL LLVM

https://reviews.llvm.org/D49573



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

[PATCH] D49338: Implement - P0122R7

[Louis Dionne via Phabricator via cfe-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

My comments are based off of 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0122r7.pdf.




Comment at: include/span:189
+struct __is_span_compatible_container<_Tp, _ElementType,
+void_t<
+// is not a specialization of span

You seem to be missing the following condition from the paper: 
`is_array_v is false`. Are you omitting it because the array 
overloads would take precedence over this constructor if an array were passed? 
If so, I think there's a bug since you could be passing an array of incorrect 
size and this constructor would kick in. The overload for `ElementType(*)[N]` 
would be discarded because of a mismatched `N`, but this one wouldn't (I think).

If I'm right, this would turn what should be a compilation failure into a 
runtime error with the `_LIBCPP_ASSERT(_Extent == _VSTD::size(__c))`.



Comment at: include/span:199
+typename enable_if<
+__is_span_compatible_ptr<
+remove_pointer_t()))>,

I would personally inline this condition for readability.



Comment at: include/span:217
+using pointer= _Tp *;
+using const_pointer  = const _Tp *; // not in standard
+using reference  = _Tp &;

Why are we providing them if they are not in the standard?



Comment at: include/span:235
+
+_LIBCPP_INLINE_VISIBILITY constexpr span(pointer __ptr, index_type 
__count) : __data{__ptr}
+{ (void)__count; _LIBCPP_ASSERT(_Extent == __count, "size mismatch in 
span's constructor (ptr, len)"); }

Those functions are specified to throw nothing in the standard. Do we want to 
add `noexcept` as a QOI thing? What's our usual stance on this?



Comment at: include/span:240
+
+_LIBCPP_INLINE_VISIBILITY constexpr span(element_type (&__arr)[_Extent])   
   : __data{__arr} {}
+_LIBCPP_INLINE_VISIBILITY constexpr span(  array& 
__arr) : __data{__arr.data()} {}

We're missing `noexcept` on these 3. They are specified as `noexcept` in the 
standard so it's not only QOI. Also, consider adding a test to catch this 
mistake.



Comment at: include/span:254
+constexpr span(const _Container& __c,
+const enable_if_t<__is_span_compatible_container::value, nullptr_t> = nullptr)
+: __data{_VSTD::data(__c)}

For both of these `Container` constructors, the paper expresses the SFINAE 
conditions based on `Container`, not on `Container` in one case and `Container 
const` in the other, which is what you're doing.

This is actually a bug in the paper, because this will make code like this 
compile:

```
std::vector const v;
std::span s(v);
```

Instead, this should be a compiler error because we're clearly not 
const-correct here, initializing a `span`-over-non-const from a const `vector`. 
Example: https://wandbox.org/permlink/kYCui3o0LEGRQ67x

This happens because we're discarding the constness of the `_Container` 
template parameter if we stick 100% to the wording of the paper. Should this be 
a DR?



Comment at: include/span:261
+constexpr span(const span<_OtherElementType, _Extent>& __other,
+   const enable_if_t<
+is_convertible_v<_OtherElementType(*)[], element_type 
(*)[]>,

`const enable_if`? Applies to the constructor below too.



Comment at: include/span:263
+is_convertible_v<_OtherElementType(*)[], element_type 
(*)[]>,
+nullptr_t> = nullptr)
+: __data{__other.data()} {}

Missing `noexcept` according to the paper. Applies to the constructor below too.



Comment at: include/span:275
+
+//  ~span() noexcept = default;
+

Why is this commented out?



Comment at: include/span:282
+static_assert(_Count >= 0);
+_LIBCPP_ASSERT(_Count <= size(), "Count out of range in 
span::first()");
+return {data(), _Count};

This can be turned into a `static_assert` since we know the extent of the span 
at compile-time.



Comment at: include/span:291
+static_assert(_Count >= 0);
+_LIBCPP_ASSERT(_Count <= size(), "Count out of range in span::last()");
+return {data() + size() - _Count, _Count};

This can be turned into a `static_assert` since we know the extent of the span 
at compile-time.



Comment at: include/span:320
+constexpr span
+inline _LIBCPP_INLINE_VISIBILITY
+   subspan(index_type __offset, index_type __count = dynamic_extent) const 
noexcept

I think we generally put those annotations before the ret

[PATCH] D49338: Implement - P0122R7

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: include/span:217
+using pointer= _Tp *;
+using const_pointer  = const _Tp *; // not in standard
+using reference  = _Tp &;

mclow.lists wrote:
> ldionne wrote:
> > Why are we providing them if they are not in the standard?
> Because (a) they're useful (see the definition of `const_iterator` below, and 
> (b) I (and STL, who wrote the final version of the `span` paper, believe that 
> not having them was just an oversight.
> 
I sent you a LWG issue email.



Comment at: include/span:254
+constexpr span(const _Container& __c,
+const enable_if_t<__is_span_compatible_container::value, nullptr_t> = nullptr)
+: __data{_VSTD::data(__c)}

mclow.lists wrote:
> ldionne wrote:
> > For both of these `Container` constructors, the paper expresses the SFINAE 
> > conditions based on `Container`, not on `Container` in one case and 
> > `Container const` in the other, which is what you're doing.
> > 
> > This is actually a bug in the paper, because this will make code like this 
> > compile:
> > 
> > ```
> > std::vector const v;
> > std::span s(v);
> > ```
> > 
> > Instead, this should be a compiler error because we're clearly not 
> > const-correct here, initializing a `span`-over-non-const from a const 
> > `vector`. Example: https://wandbox.org/permlink/kYCui3o0LEGRQ67x
> > 
> > This happens because we're discarding the constness of the `_Container` 
> > template parameter if we stick 100% to the wording of the paper. Should 
> > this be a DR?
> Yes, this should be an LWG issue.
Ugh, actually, I am wrong! The paper is fine and you implemented the paper 
correctly. The paper says

```
Remarks: These constructors shall not participate in overload resolution unless:
— `Container` is not a specialization of span,
— `Container` is not a specialization of array,
— `is_array_v` is `false`,
— `data(cont)` and `size(cont)` are both well-formed, and
— `remove_pointer_t(*)[]` is convertible to 
`ElementType(*)[]`.
```

since `cont` has the right constness, the paper properly describes what should 
happen. My mistake.




Comment at: include/span:275
+
+//  ~span() noexcept = default;
+

mclow.lists wrote:
> ldionne wrote:
> > Why is this commented out?
> This is commented out because of a clang bug 
> https://bugs.llvm.org/show_bug.cgi?id=38143, where copy constructor will 
> become non-constexpr.  Once this fixed, we can un-comment this line (though 
> it should make no difference if it is present or not).
> 
What! Thanks.



Comment at: include/span:351
+{
+pointer __p = __data;
+__data = __other.__data;

mclow.lists wrote:
> ldionne wrote:
> > Just curious -- why not use `_VSTD::swap(__data, __other.__data)`? This 
> > would avoid any potential for a stupid logic error to sneak up.
> Not to have to include ``? (It may get included anyway)
`` already includes ``. Your choice, I don't mind.



Comment at: include/span:356-359
+_LIBCPP_INLINE_VISIBILITY span 
__as_bytes() const noexcept
+{ return {reinterpret_cast(data()), size_bytes()}; }
+
+_LIBCPP_INLINE_VISIBILITY span 
__as_writeable_bytes() const noexcept

mclow.lists wrote:
> ldionne wrote:
> > It looks like neither `as_bytes` nor `as_writeable_bytes` is marked `const` 
> > in the paper. Why are we deviating?
> In N4762, they're marked as non-member functions that take `span` 
> by value. This implementation just turns around and calls a member function 
> (with an unpronounceable name) to do the work.  The standard never mentions 
> `__as_bytes` or `__as_writeable_bytes`.
> 
> 
> 
> 
> 
I completely missed the leading underscores on those methods, along with the 
non-member functions. Thanks, this is not an issue.



Comment at: include/span:531
+operator==(const span<_Tp1, _Extent1>& __lhs, const span<_Tp2, _Extent2>& 
__rhs)
+{ return equal(__lhs.begin(), __lhs.end(), __rhs.begin(), __rhs.end()); }
+

mclow.lists wrote:
> ldionne wrote:
> > It's kind of crazy those are not constrained in any way, but that's what 
> > the paper says. I would expect some constraint based on whether we can 
> > compare `_Tp1` and _Tp2`.
> In my prototype implementation, they were constrained. But LEWG decided not 
> to go there (which is the same approach taken in https://wg21.link/P0805 ). 
> There needs to be another paper written about constraining container 
> comparisons.
> 
> It also applies to homogenous comparisons. Consider `vector>` 
>  Two of them can't be compared (less than, etc), because `complex` 
> doesn't have a `operator<`
Thanks for the information.


https://reviews.llvm.org/D49338



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

[PATCH] D49573: [CMake] Option to control whether shared/static library is installed

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Ah, so you mean you need it for libc++abi because you want to build them but 
not install them (so they're linked into libc++)? Then LGTM as-is. I'd like to 
see what Eric has to say though.


Repository:
  rL LLVM

https://reviews.llvm.org/D49573



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

[PATCH] D49338: Implement - P0122R7

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM, with a suggestion for adding one test.




Comment at: include/span:189
+struct __is_span_compatible_container<_Tp, _ElementType,
+void_t<
+// is not a specialization of span

ldionne wrote:
> You seem to be missing the following condition from the paper: 
> `is_array_v is false`. Are you omitting it because the array 
> overloads would take precedence over this constructor if an array were 
> passed? If so, I think there's a bug since you could be passing an array of 
> incorrect size and this constructor would kick in. The overload for 
> `ElementType(*)[N]` would be discarded because of a mismatched `N`, but this 
> one wouldn't (I think).
> 
> If I'm right, this would turn what should be a compilation failure into a 
> runtime error with the `_LIBCPP_ASSERT(_Extent == _VSTD::size(__c))`.
Consider adding a test for this.


https://reviews.llvm.org/D49338



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

[PATCH] D49711: [CMake] Fix the setting of LIBCXX_HEADER_DIR in standalone build

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

LGTM. FWIW, I'm really not a huge fan of all these different ways of building 
libc++. There should be one canonical way of doing it, and it should be simple. 
However, it seems like this patch is the right thing to do given the changes 
that have already been made to account for specific use cases.

IOW, I don't like this whole trend, but we're already down that road.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49711



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

[PATCH] D49774: [libc++] Use __int128_t to represent file_time_type.

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49774#1174588, @EricWF wrote:

> In https://reviews.llvm.org/D49774#1174565, @mclow.lists wrote:
>
> > I haven't reviewed this closely, but you might want to look at 
> > http://wg21.link/P0355, where we added a `file_clock` and `file_time` types.
>
>
> Thanks for the information. It doesn't look to have too much bearing on this 
> from a design standpoint. Except to note that it seems to solve
>  the streaming problem by adding explicit streaming overload for time points.


It doesn't change anything from a design standpoint, but I don't think we can 
ship something deemed stable without taking P0355 into account. That's because 
it adds `file_clock` and changes `file_time_type` to use it, which is an ABI 
break. So if we take `filesystem` out of `experimental` and ship it before 
we've actually implemented the parts of P0355 that change the ABI, we'll have 
to take an ABI break in the future. That would suck because this ABI break 
would be necessary to implement C++20 properly in a fairly major way.

That's my understanding. Appart from that, I've read the design docs and while 
I'm not very familiar with the problems involved, I follow the reasoning and I 
think using `__int128_t` makes at least as much sense as the other potential 
solutions.

LGTM


Repository:
  rCXX libc++

https://reviews.llvm.org/D49774



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

[PATCH] D49774: [libc++] Use __int128_t to represent file_time_type.

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49774#1175131, @mclow.lists wrote:

> In https://reviews.llvm.org/D49774#1175062, @ldionne wrote:
>
> > In https://reviews.llvm.org/D49774#1174588, @EricWF wrote:
> >
> > > In https://reviews.llvm.org/D49774#1174565, @mclow.lists wrote:
> > >
> > > > I haven't reviewed this closely, but you might want to look at 
> > > > http://wg21.link/P0355, where we added a `file_clock` and `file_time` 
> > > > types.
> > >
> > >
> > > Thanks for the information. It doesn't look to have too much bearing on 
> > > this from a design standpoint. Except to note that it seems to solve
> > >  the streaming problem by adding explicit streaming overload for time 
> > > points.
> >
> >
> > It doesn't change anything from a design standpoint, but I don't think we 
> > can ship something deemed stable without taking P0355 into account. That's 
> > because it adds `file_clock` and changes `file_time_type` to use it, which 
> > is an ABI break. So if we take `filesystem` out of `experimental` and ship 
> > it before we've actually implemented the parts of P0355 that change the 
> > ABI, we'll have to take an ABI break in the future. That would suck because 
> > this ABI break would be necessary to implement C++20 properly in a fairly 
> > major way.
>
>
> Another problem (that Eric and I discussed last night) is that filesystem is 
> part of C++17, and `file_clock` is C++20. So we need a solution for C++17 as 
> well.


I believe one approach here would be to define `__file_clock` in C++17 (the way 
it is specified in C++20), and then have `using file_clock = __file_clock` in 
C++20. Would this be a valid strategy?


Repository:
  rCXX libc++

https://reviews.llvm.org/D49774



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

[PATCH] D49808: [libc++] Factor duplicate code into function templates

[Louis Dionne via Phabricator via cfe-commits]

ldionne created this revision.
ldionne added a reviewer: mclow.lists.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, dexonsmith, christof.

The exact same code was replicated 11 times for implementing the basic_istream
input operators (those that don't use numeric_limits). The same code was also
duplicated twice for implementing the basic_istream input operators that take
numeric_limits into account.

This commit factors the common code into function templates to avoid
the duplication.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49808

Files:
  libcxx/include/istream

Index: libcxx/include/istream
===
--- libcxx/include/istream
+++ libcxx/include/istream
@@ -358,381 +358,162 @@
 {
 }
 
-template 
+template 
+_LIBCPP_INLINE_VISIBILITY
 basic_istream<_CharT, _Traits>&
-basic_istream<_CharT, _Traits>::operator>>(unsigned short& __n)
-{
+__input_arithmetic(basic_istream<_CharT, _Traits>& __is, _Tp& __n) {
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
+typename basic_istream<_CharT, _Traits>::sentry __s(__is);
 if (__s)
 {
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
+typedef istreambuf_iterator<_CharT, _Traits> _Ip;
+typedef num_get<_CharT, _Ip> _Fp;
 ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
+use_facet<_Fp>(__is.getloc()).get(_Ip(__is), _Ip(), __is, __err, __n);
+__is.setstate(__err);
 }
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
 {
-this->__set_badbit_and_consider_rethrow();
+__is.__set_badbit_and_consider_rethrow();
 }
 #endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return __is;
+}
+
+template 
+basic_istream<_CharT, _Traits>&
+basic_istream<_CharT, _Traits>::operator>>(unsigned short& __n)
+{
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(unsigned int& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(unsigned long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(long long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_conside

[PATCH] D49808: [libc++] Factor duplicate code into function templates

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX337955: [libc++] Factor duplicate code into function 
templates (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49808?vs=157316&id=157340#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D49808

Files:
  include/istream

Index: include/istream
===
--- include/istream
+++ include/istream
@@ -358,381 +358,162 @@
 {
 }
 
-template 
+template 
+_LIBCPP_INLINE_VISIBILITY
 basic_istream<_CharT, _Traits>&
-basic_istream<_CharT, _Traits>::operator>>(unsigned short& __n)
-{
+__input_arithmetic(basic_istream<_CharT, _Traits>& __is, _Tp& __n) {
 #ifndef _LIBCPP_NO_EXCEPTIONS
 try
 {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
+typename basic_istream<_CharT, _Traits>::sentry __s(__is);
 if (__s)
 {
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
+typedef istreambuf_iterator<_CharT, _Traits> _Ip;
+typedef num_get<_CharT, _Ip> _Fp;
 ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
+use_facet<_Fp>(__is.getloc()).get(_Ip(__is), _Ip(), __is, __err, __n);
+__is.setstate(__err);
 }
 #ifndef _LIBCPP_NO_EXCEPTIONS
 }
 catch (...)
 {
-this->__set_badbit_and_consider_rethrow();
+__is.__set_badbit_and_consider_rethrow();
 }
 #endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return __is;
+}
+
+template 
+basic_istream<_CharT, _Traits>&
+basic_istream<_CharT, _Traits>::operator>>(unsigned short& __n)
+{
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(unsigned int& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(unsigned long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(long long& __n)
 {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
-{
-#endif  // _LIBCPP_NO_EXCEPTIONS
-sentry __s(*this);
-if (__s)
-{
-typedef istreambuf_iterator _Ip;
-typedef num_get _Fp;
-ios_base::iostate __err = ios_base::goodbit;
-use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __n);
-this->setstate(__err);
-}
-#ifndef _LIBCPP_NO_EXCEPTIONS
-}
-catch (...)
-{
-this->__set_badbit_and_consider_rethrow();
-}
-#endif  // _LIBCPP_NO_EXCEPTIONS
-return *this;
+return _VSTD::__input_arithmetic(*this, __n);
 }
 
 template 
 basic_istream<_CharT, _Traits>&
 basic_istream<_CharT, _Traits>::operator>>(unsigned long long& __n)
 {
-#ifndef _LIBCPP_NO

[PATCH] D49774: [libc++] Use __int128_t to represent file_time_type.

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49774#1175543, @BillyONeal wrote:

> In https://reviews.llvm.org/D49774#1175131, @mclow.lists wrote:
>
> > Another problem (that Eric and I discussed last night) is that filesystem 
> > is part of C++17, and `file_clock` is C++20. So we need a solution for 
> > C++17 as well.
>
>
> It seems like we need to fix C++20 to allow that to be a typedef to a type in 
> std::filesystem or that will be ABI breaking for MSVC++. IMO we should fix 
> the spec to allow that rather than make libc++ jump through insane hoops.


We could also just provide `file_clock` "early" in C++17. Strictly speaking 
that may make our implementation non-conforming, but IDK how big of a deal this 
would be?


Repository:
  rCXX libc++

https://reviews.llvm.org/D49774



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