[PATCH] D58289: [clang] Only provide C11 features in starting with C++17

This revision was automatically updated to reflect the committed changes. Closed by commit rL354691: [clang] Only provide C11 features in starting with C++17 (authored by ldionne, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to com

[PATCH] D64089: [Driver] Introduce -stdlib++-isystem

ldionne added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1307 + HasStdlibxxIsystem ? TC.AddClangCXXStdlibIsystemArgs(Args, CmdArgs) + : TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs); +}); So, b

[PATCH] D48680: Add missing visibility annotation for __base

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Herald added a subscriber: dexonsmith. @pcc In your reproducer, what is `~/l3/ra-cmake/bin/clang++`? Are you able to reproduce without `-fuse-ld=lld`? I'm trying to reproduce locall

[PATCH] D64089: [Driver] Introduce -stdlib++-isystem

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. This looks fine to me, but I'd like to have someone else familiar with the Driver/Frontend design sign off too. The implementation is trivial but I'd li

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

ldionne added a comment. In D66364#1635814 , @aaron.ballman wrote: > [ ...] > > Adding some libc++ maintainers to see if they have opinions. > > `__extension__` is one option. Could we get away with push/pop disabling of > the diagnostic? Or perhaps this

[PATCH] D43159: Modernize: Use nullptr more.

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. I'm fine with this given what the author said about `nullptr` already being used in those headers. Repository: rCXX libc++ CHANGES SINCE LAST ACTIO

[PATCH] D43159: Modernize: Use nullptr more.

ldionne requested changes to this revision. ldionne added a comment. I agree with Marshall's requests. Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43159/new/ https://reviews.llvm.org/D43159 ___ cfe-commits maili

[PATCH] D43159: Modernize: Use nullptr more.

ldionne added inline comments. Comment at: include/__threading_support:323 bool __libcpp_thread_isnull(const __libcpp_thread_t *__t) { - return *__t == 0; + return *__t == nullptr; } mclow.lists wrote: > mclow.lists wrote: > > This one is wrong. > `__libcpp_t

[PATCH] D64383: build: use multiple `install` rather than building up a list

ldionne added a comment. I like that. Almost anything that helps us abolish global variables and lists (here `LIBCXX_INSTALL_TARGETS`) in the CMake scripts is a good thing. Comment at: src/CMakeLists.txt:441 + if (LIBCXX_INSTALL_STATIC_LIBRARY) +install(TARGETS cxx_static

[PATCH] D64089: [Driver] Introduce -stdlib++-isystem

ldionne added a comment. Background question: I'm familiar with the behavior on Darwin, where we prefer C++ headers in the toolchain (alongside the driver), however do other platforms behave the same? I thought this was something specific to Darwin. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D61858: Make `__is_base_of` more friendly with unions

ldionne added a comment. Shouldn't we also add unit tests for PR41843 in libc++? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61858/new/ https://reviews.llvm.org/D61858 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D61858: Make `__is_base_of` more friendly with unions

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. I'm not a frontend expert, but this looks reasonable to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61858/new/ https://reviews.llvm.org/D61858 _

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

ldionne added a comment. > When the insert(P&&) is called, it delegates to emplace, which only gets > Cpp17EmplaceConstructible from the supplied parameters, not > Cpp17EmplaceConstructible from add_const_t of the parameters. Thus, libcxx's > test is asserting a condition the standard explicitl

[PATCH] D61963: [clang][Darwin] Refactor header search path logic into the driver

ldionne created this revision. ldionne added reviewers: jfb, arphaman. Herald added subscribers: cfe-commits, jsji, dexonsmith, jkorous, christof, kbarton, javed.absar, nemanjai. Herald added a project: clang. This commit moves the logic for determining system, resource and C++ header search path

[PATCH] D61963: [clang][Darwin] Refactor header search path logic into the driver

ldionne updated this revision to Diff 200311. ldionne added a comment. Fix test that fails on systems that don't have /usr/include/c++. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61963/new/ https://reviews.llvm.org/D61963 Files: clang/include

[PATCH] D61963: [clang][Darwin] Refactor header search path logic into the driver

This revision was automatically updated to reflect the committed changes. Closed by commit rC361278: [clang][Darwin] Refactor header search path logic into the driver (authored by ldionne, committed by ). Changed prior to commit: https://reviews.llvm.org/D61963?vs=200311&id=200539#toc Reposito

[PATCH] D62268: Add back --sysroot support for darwin header search.

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. This LGTM. When I did the refactor, all the code was only using `-isysroot` (and never accessing `--sysroot`), so I thought only `-isysroot` was relevant on Darwin. Seems like I was wrong.

[PATCH] D62493: [Driver] Always use Unix-style paths in the Darwin driver

ldionne created this revision. ldionne added reviewers: jfb, rsmith. Herald added subscribers: cfe-commits, dexonsmith, jkorous. Herald added a project: clang. Otherwise, the unit tests seem to be confused on Windows. Note that it would in theory be better to always use the platform's path, howeve

[PATCH] D62493: [Driver] Always use Unix-style paths in the Darwin driver

ldionne added a comment. A few notes: - I did setup a Windows machine and run the tests on it - I'd welcome a better way to solve this problem than ripping out the use of `llvm::sys::path::append`, but I wasn't able to find anything in `FileCheck` that would do it Repository: rG LLVM Github

[PATCH] D48680: Add missing visibility annotation for __base

ldionne added a comment. What I don't understand is under which circumstances this changes anything, since we don't export `__base` from the dylib, and implicit instantiations of `__base` don't cause it to be exported. Can you please provide a sample program where this changes what's exported,

[PATCH] D48680: Add missing visibility annotation for __base

ldionne added a comment. More specifically, the following program always causes the vtable for `__base` to be internal: cat < int go(float) { return 0; } std::function foo() { return go; } int main() { foo()(3.3f); } EOF That's true with or without this patch, and with or withou

[PATCH] D62493: [Driver] Always use Unix-style paths in the Darwin driver

ldionne added a comment. In D62493#1519708 , @rnk wrote: > So, there's nothing wrong, functionally speaking, with what we do today, > right? It's just inconvenient to test. What we do for Darwin today (without this patch) is functionally correct, but h

[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

ldionne accepted this revision. ldionne added a comment. LGTM, but I suggest you address @Quuxplusone 's concerns regarding the tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits

[PATCH] D56445: [Sema] Teach Clang that aligned allocation is not supported with macosx10.13

ldionne created this revision. ldionne added a reviewer: ahatanak. Herald added subscribers: cfe-commits, dexonsmith, jkorous, christof. r306722 added diagnostics when aligned allocation is used with deployment targets that do not support it, but the first macosx supporting aligned allocation was

[PATCH] D56445: [Sema] Teach Clang that aligned allocation is not supported with macosx10.13

This revision was automatically updated to reflect the committed changes. Closed by commit rL350649: [Sema] Teach Clang that aligned allocation is not supported with macosx10.13 (authored by ldionne, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revie

[PATCH] D50106: [libc++] Fix tuple assignment from type derived from a tuple-like

ldionne added a comment. Herald added subscribers: libcxx-commits, jkorous. Ping! It would be nice to land this in LLVM 8 Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50106/new/ https://reviews.llvm.org/D50106 __

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne added a comment. LGTM. All comments/questions are just for my education. Other things I did: double-check that you changed all the functions changed by https://cplusplus.github.io/LWG/lwg-defects.html#2946. Comment at: include/memory:5647 + typename __void_t::ty

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne added a comment. More missed `noexcept`s. Comment at: include/string:279 +template +size_type find_first_of(const T& t, size_type pos = 0) const noexcept; // C++17 size_type find_first_of(const value_type* s, size_type pos, size_type n) const noexcep

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne added inline comments. Comment at: include/string:842 +explicit basic_string(const _Tp& __t, + typename enable_if<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value, void>::type* = 0); + mclow.lists wrote: > ldion

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne added inline comments. Comment at: include/string:836 _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS -basic_string(const _Tp& __t, size_type __pos, size_type __n, - const allocator_type& __a = allocator_type(), -

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne added inline comments. Comment at: include/string:856 +_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS +explicit basic_string(const _Tp& __t, const allocator_type& __a); + tcanens wrote: > ldionne wrote: > > I think this `explicit` shou

[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

ldionne added a comment. I guess I'm echoing some of Eric's comments, but does it make sense to build libc++abi against a system-installed libc++? If not, this change makes sense. Repository: rCXXA libc++abi https://reviews.llvm.org/D48694 ___ c

[PATCH] D48616: Implement LWG 2946, 3075 and 3076

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D48616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

ldionne added a comment. In https://reviews.llvm.org/D48694#1148718, @EricWF wrote: > As an aside, libc++abi should really live in the libc++ repository. That way > it would always have the correct headers available. But every time I pitch > that idea I get a ton of push back. FWIW, I think I

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

This revision was automatically updated to reflect the committed changes. Closed by commit rCXX336369: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY (authored by ldionne, committed by ). Herald added a subscriber: cfe-commits. Changed prior to commit: https://revie

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

ldionne added a comment. I reverted this commit. Sorry for the blunder. I'll take a look at why LLDB's tests are doing this. Repository: rCXX libc++ https://reviews.llvm.org/D48892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

ldionne updated this revision to Diff 154415. ldionne added a comment. This revision to the patch fixes a problem where __pbump had been applied _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY instead of _LIBCPP_INLINE_VISIBILITY, which caused the symbols exported in the ABI to change and broke the CI.

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

ldionne added inline comments. Comment at: libcxx/include/streambuf:261 -_LIBCPP_ALWAYS_INLINE +_LIBCPP_INLINE_VISIBILITY void __pbump(streamsize __n) { __nout_ += __n; } This one was marked as `_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY` instead of

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

ldionne added a comment. I've now managed to run the `check-cxx-abilist` test on my machine and it passes. I'd like to commit this again, @EricWF am I good to go? Repository: rL LLVM https://reviews.llvm.org/D48892 ___ cfe-commits mailing list c

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Generally looks pretty good, but I have a couple questions/suggestions. Comment at: libcxx/include/__hash_table:2165 +#if _LIBCPP_STD_VER > 14 +template +templat

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

ldionne added a comment. I forgot to mention it in my initial review, but it also seems like you forgot to update all the synopsis in the comments at the beginning of headers. https://reviews.llvm.org/D46845 ___ cfe-commits mailing list cfe-commits

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

ldionne added a comment. Pushing since I got an offline O.K. from Eric. Repository: rL LLVM https://reviews.llvm.org/D48892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL336866: [libc++] Take 2: Replace uses of _LIBCPP_ALWAYS_INLINE by… (authored by ldionne, committed by ). Changed prior to

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rCXX336866: [libc++] Take 2: Replace uses of _LIBCPP_ALWAYS_INLINE by… (authored by ldionne, committed by ). Changed prior

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

ldionne added inline comments. Comment at: libcxx/include/map:43 typedef std::reverse_iteratorconst_reverse_iterator; +typedef unspecified node_type; +typedef INSERT_RETURN_TYPE insert_return_type; You are missing `/

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. LGTM -- great! Just make sure the commit message does not pretend the change includes `merge`. This may be obvious, but my LGTM alone should probably not be enough to submit. https://revie

[PATCH] D45805: [libcxx] Remove redundant specializations in type_traits.

ldionne added a subscriber: mclow.lists. ldionne added inline comments. Comment at: include/type_traits:595 -template -struct __and_<_B0, _B1> : conditional<_B0::value, _B1, _B0>::type {}; - It is not impossible that this was a compile-time optimization for the

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

ldionne added inline comments. Comment at: libcxx/CMakeLists.txt:158 +cmake_dependent_option(LIBCXX_ENABLE_STATIC_ABI_LIBRARY_STATIC + "Statically link the ABI library to static library" ON phosek wrote: > Maybe `LIBCXX_ENABLE_STATIC_ABI_LIBRARY_IN_STATIC_LIBR

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne added a comment. ABI-wise, I think this change is OK. Indeed, if `__type_visibility__` is not supported, we're already marking the base template as `__visibility__("default")`, so marking the extern template declaration with `__visibility__("default")` is not a problem. If `__type_visib

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne added a comment. In https://reviews.llvm.org/D35388#1167315, @thomasanderson wrote: > In https://reviews.llvm.org/D35388#1167305, @ldionne wrote: > > > ABI-wise, I think this change is OK. Indeed, if `__type_visibility__` is > > not supported, we're already marking the base template as

[PATCH] D68681: [libc++][test] Miscellaneous MSVC cleanups

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. You have commit access, right? If so, go ahead. Otherwise, LMK and I can commit this for you. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68681/new/ https://reviews.llvm.org/D686

[PATCH] D69062: Resolve LWG issue 2426

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. If we want to mark the LWG issue as being resolved for libc++, I think the test should be moved to the libc++ test suite. Otherwise, we're only assessing that libc++ implements LWG

[PATCH] D69221: [clang][darwin] Fix search path logic for C_INCLUDE_DIRS

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Good find! The following drivers (at least) appear to have the same problem: clang/lib/Driver/ToolChains/WebAssembly.cpp:236 clang/lib/Driver/ToolChains/Solaris.cpp:247 clang/

[PATCH] D69363: [www] Change URLs to HTTPS.

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Nice! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69363/new/ https://reviews.llvm.org/D69363 ___ cfe-commits

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

ldionne added a comment. In D71726#1791904 , @jfb wrote: > This generally seems fine. Does it work on most backends? I want to make sure > it doesn't fail in backends :) > > Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for > floatin

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

ldionne added inline comments. Comment at: clang/test/CodeGen/atomic-ops.c:296 + // CHECK: fsub + return __atomic_sub_fetch(p, 1.0, memory_order_relaxed); +} Sorry if that's a dumb question, but I'm a bit confused: `p` is a `float*`, but then we add a double

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

ldionne added a comment. Herald added subscribers: mstorsjo, dexonsmith. Note: this commit was re-reverted in commit bbb2655de0049f8e6cf4482702aa616011c851c1 Author: Richard Smith Date: Mon May 21 20:10:54 2018 + Revert r332028; see PR37545 for details. llvm-svn: 33

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

ldionne added a comment. I'm revisiting http://llvm.org/PR37398 as it looks like a pretty serious bug to me. I read http://llvm.org/D46665, this patch and http://llvm.org/PR37545, and my understanding is that this patch supersedes @EricWF 's original patch http://llvm.org/D46665. The only rema

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

ldionne added inline comments. Comment at: clang/test/CodeGen/atomic-ops.c:296 + // CHECK: fsub + return __atomic_sub_fetch(p, 1.0, memory_order_relaxed); +} yaxunl wrote: > ldionne wrote: > > Sorry if that's a dumb question, but I'm a bit confused: `p` is a

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

ldionne added a comment. In D47092#2049842 , @efriedma wrote: > I think the preferred solution is something like > https://bugs.llvm.org/show_bug.cgi?id=37545#c4, which is slightly different > from both this patch and D46665

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

ldionne added inline comments. Comment at: clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu:26 + // CHECK: atomicrmw fsub double* {{.*}} monotonic + return __atomic_fetch_sub(p, 1.0, memory_order_relaxed); +} Nitpick, but this should be `1.0L` to be consistent. CH

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

ldionne added inline comments. Comment at: libcxx/trunk/include/__split_buffer:201 __alloc_rr& __a = this->__alloc(); +pointer __to_be_end = this->__end_; do danlark wrote: > hiraditya wrote: > > danlark wrote: > > > lichray wrote: > > > > mclow.lis

[PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

This revision was automatically updated to reflect the committed changes. Closed by commit rGafa1afd4108d: [CMake] Bump CMake minimum version to 3.13.4 (authored by ldionne). Changed prior to commit: https://reviews.llvm.org/D78648?vs=259300&id=279895#toc Repository: rG LLVM Github Monorepo

[PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

ldionne added a comment. Herald added subscribers: llvm-commits, msifontes, sstefan1, jurahul, stephenneuendorffer, aartbik. Herald added projects: MLIR, LLVM. Okay, the previous patch has landed and no issues have come up, so I'm going to move forward with this patch now. Repository: rG LLV

[Differential] D78648: [CMake] Bump CMake minimum version to 3.13.4

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGafa1afd4108d: [CMake] Bump CMake minimum version to 3.13.4 (authored by ldionne). Herald added subscribers: llvm-commits, msifontes, sstefan1, jurahu

[PATCH] D73245: Avoid using std::max_align_t in pre-C++11 mode

ldionne accepted this revision. ldionne added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73245/new/ https://reviews.llvm.org/D73245 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Thanks for looking into this! Comment at: clang/test/SemaObjC/type-traits-is-pointer.mm:10 +void test_is_pointer() { +assert_is_pointer(); +

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

ldionne added inline comments. Comment at: clang/test/SemaObjCXX/type-traits-is-pointer.mm:1 +// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -verify %s +// expected-no-diagnostics Why do you run this through `-verify`? Can't you just drop tha

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

ldionne accepted this revision. ldionne added inline comments. This revision is now accepted and ready to land. Comment at: libcxx/include/type_traits:901 +// In clang 10.0.0 and earlier __is_pointer didn't work with Objective-C types. +#if __has_keyword(__is_pointer) && _LIBCPP_

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

ldionne added inline comments. Comment at: clang/test/SemaObjCXX/type-traits-is-pointer.mm:1 +// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -verify %s +// expected-no-diagnostics ldionne wrote: > Why do you run this through `-verify`? Can't

[PATCH] D77697: libc++: adjust modulemap for non-modular C

ldionne added a comment. In D77697#1968998 , @joerg wrote: > This fixes the module build of clang for me. @joerg When did it start failing for you? And what part of it did? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

ldionne added inline comments. Comment at: libcxx/include/type_traits:901 +// In clang 10.0.0 and earlier __is_pointer didn't work with Objective-C types. +#if __has_keyword(__is_pointer) && _LIBCPP_CLANG_VER > 1000 + zoecarver wrote: > ldionne wrote: > > zoecarv

[PATCH] D81622: [Clang] Search computed sysroot for libc++ header paths

ldionne added a comment. While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used onl

[PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

ldionne added a comment. This review is for the upcoming CMake upgrade once we branch off the LLVM 11 release branch. I won't apply it until after the branch has been created, which is around July. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D786

[PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

ldionne created this revision. Herald added subscribers: libcxx-commits, openmp-commits, lldb-commits, Sanitizers, cfe-commits, frgossen, grosul1, jvesely, Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, jpienaar, rriddle, mehdi_amini, lebedev.ri, de

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

ldionne added a comment. I personally don't think the note is useful. What we're trying to tell the user is "don't use X after you've moved out of X" -- whether the move actually has an effect or not is not useful information. To me, adding `; move of a 'const' argument has no effect` just make

[PATCH] D78762: build: use `find_package(Python3)` if available

ldionne accepted this revision. ldionne added a comment. In D78762#2002305 , @JDevlieghere wrote: > My suggestion is to have 4 new CMake variable that abstract over this: > `LLVM_PYTHON_EXECUTABLE`, `LLVM_PYTHON_LIBRARIES`, `LLVM_PYTHON_INCLUDE_DIRS` >

[PATCH] D78944: [libc++][test] Disable test for extension that's unsupportable in C++20

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Could you add a short comment explaining the `#if`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78944/new/ https://reviews.llvm.or

[PATCH] D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast

ldionne added a comment. Gentle ping. This is blocking the implementation of `std::bit_cast` in libc++. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76323/new/ https://reviews.llvm.org/D76323 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D85924: [clang] Add __RELATIVE_CXX_ABI_VTABLES predefine macro

ldionne added inline comments. Comment at: clang/lib/Frontend/InitPreprocessor.cpp:1127 + if (LangOpts.RelativeCXXABIVTables) +Builder.defineMacro("__RELATIVE_CXX_ABI_VTABLES"); Should we be using some `__has_feature(...)` instead? Repository: rG LLVM

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. This change LGTM, libc++ will keep working as-is since we assume the builtins return `bool` (I checked). CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D79343: [libc++][test] Adjust move_iterator tests to allow C++20

ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we ca

[PATCH] D77697: libc++: adjust modulemap for non-modular C

ldionne accepted this revision as: libc++. ldionne added a comment. LGTM since other reviewers are fine with it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77697/new/ https://reviews.llvm.org/D77697 ___

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

ldionne added a comment. What name and email do you want this committed with? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79504/new/ https://reviews.llvm.org/D79504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llv

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

ldionne added a comment. I'll commit this since I think everyone was fine with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79504/new/ https://reviews.llvm.org/D79504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

This revision was automatically updated to reflect the committed changes. Closed by commit rG9aee35bcc90f: [Clang] Fix the incorrect return type of atomic_is_lock_free (authored by kamleshbhalui, committed by ldionne). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D27434: [libc++abi] Disable failing test on Darwin

ldionne added a comment. Note for posterity in case someone does some archeology and ends up here (as I am doing): the Radar number is 29523281. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D27434/new/ https://reviews.llvm.org/D27434 __

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

ldionne added a comment. @Tyker @rsmith You might be interested in reading https://llvm.org/PR45912, which describes what I believe to be an issue with this patch (or another related patch for `consteval` -- not sure). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D69520: [libc++] Disallow dynamic -> static span conversions

ldionne commandeered this revision. ldionne edited reviewers, added: jdoerrie; removed: ldionne. ldionne added a comment. Herald added subscribers: broadwaylamb, jkorous. In D69520#1878360 , @miscco wrote: > I believe this is superseded by the implementati

[PATCH] D133249: [libc++] Documents details of the pre-commit CI.

ldionne added a comment. This is awesome. I have some comments, but I have not done an in-depth pass at this yet. Comment at: libcxx/docs/Contributing.rst:139 + +.. [#] There's `Dev meeting talk `__ + explaining the benefits of l

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

ldionne added a comment. Wouldn't re-applying https://github.com/llvm/llvm-project/commit/fcd549a7d8284a8e7c763fee3da2206acd8cdc4f (which had been reverted IIUC) be a more precise fix for this problem? We'd suppress the warning, but only for classes that we know are OK to use with CTAD. It is

[PATCH] D132608: [CMake] Clean up CMake binary dir handling

ldionne added a comment. I think I like the spirit of this change, which seems to be to move us closer to `CMAKE_foo` variables and further from `LLVM_foo` variables for equivalent functionality. I have a comment, but this essentially LGTM. The libc++ CI failures (in particular the bootstrappin

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

ldionne added a comment. In D126907#3746477 , @Mordante wrote: > Unfortunately there are a lot of different options and combination of options > to test libc++. > So it's indeed not possible to test all options with once check-cxx > invocation. This ^.

[PATCH] D133249: [libc++] Documents details of the pre-commit CI.

ldionne added inline comments. Comment at: clang/www/hacking.html:295-296 + directory will cause the update of the diff to start a CI run. This dummy + file will also add the libc++ group to the list of reviewers. The status of + the build will be available in Phabricator. + -

[PATCH] D130867: [clang] adds builtin `std::invoke` and `std::invoke_r`

ldionne added a comment. I think this is quite interesting, but it also raises the obvious question of: where do we stop? I think the following elements need to be taken into account: 1. Implementing more stuff in the compiler adds complexity to the compiler, and does not decrease the complexit

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

ldionne added a comment. In D133425#3777598 , @aaron.ballman wrote: > In D133425#3775353 , @ldionne wrote: > >> Wouldn't re-applying >> https://github.com/llvm/llvm-project/commit/fcd549a7d8284a8e7c763fee3da2206

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

ldionne added a comment. FWIW I just created https://reviews.llvm.org/D133535. IMO that is a safer alternative, in the sense that we only opt-in those types that we know are safe to use with CTAD, but the remaining classes still get a warning. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ldionne added a comment. Just my .02, but I am conflicted between: 1. Simply not doing anything -- the diagnostic users get when they violate the requirement currently is probably not that bad? I did see this breakage a bit in our internal code base as well, but it was easy to fix and there wer

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

ldionne added a comment. In D133425#3779121 , @dblaikie wrote: >> One thing I don't understand in the current state of things is why the >> diagnostic fires at all inside system headers. I thought warnings in system >> headers were discarded? > > It doe

[PATCH] D130867: [clang] adds builtin `std::invoke` and `std::invoke_r`

ldionne added a comment. In D130867#3781429 , @cjdb wrote: > I had not realised that libc++ was listed as a reviewer. The change does have an impact on libc++, so I think it makes sense to look at it not only from the compiler perspective, but from the

<    1   2   3   4   5   6   7   8   >