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
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=15
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
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 sub
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 sup
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 func
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
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/listinf
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
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_alloca
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
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
--
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-
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
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 t
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
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-comm
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
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++
> > > head
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.pas
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 consisten
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
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 befor
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
> > >
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
exp
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/m
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 i
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
==
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 thi
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.suppor
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/D5034
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
> >
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.su
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.pa
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 `XF
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
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.avai
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=160
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
htt
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 us
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.
Rep
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:/
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 a
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:
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:
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 \
> > intMa
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.
R
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__`
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
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
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 l
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.llv
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
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
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/cf
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 li
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
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://revi
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); }
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:
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 n
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:
rC
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=1
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, wh
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
__
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
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=
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:
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 m
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/piper
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
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(Te
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,
Sta
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/Se
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://review
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
Reposi
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/cl
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.llv
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
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
> > > de
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/clan
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))
--
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 instr
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 inst
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
Reposito
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))
--
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
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
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 lik
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-
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_conta
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 w
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.
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 sp
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 acco
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` typ
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:
> > >
>
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).
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:
rCX
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
1 - 100 of 780 matches
Mail list logo
