[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

Quuxplusone added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530 + // Don't need to mark Objective-C methods or blocks since the undefined + // behaviour optimization isn't used for them. +} This seems like a trap waiting to spring on

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

Quuxplusone added inline comments. Comment at: include/clang/Sema/AnalysisBasedWarnings.h:93 - void IssueWarnings(Policy P, FunctionScopeInfo *fscope, - const Decl *D, const BlockExpr *blkExpr); + void IssueWarnings(Policy P, FunctionScopeInfo *fscope,

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

Quuxplusone added a comment. The provided example (typoing "i" for "j") sounds like the sort of thing that PVS-Studio catches; maybe see what wording they use for that kind of mistake? Without investigating, I would suggest "cut-and-paste-error" or "likely-typo". However, the attached patch

[PATCH] D28467: [Sema] Add warning for unused lambda captures

Quuxplusone added inline comments. Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17 + auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}} + auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; //

[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer

Quuxplusone added subscribers: rjmccall, Quuxplusone. Quuxplusone added a comment. Re the language option: In https://reviews.llvm.org/D27163, @rsmith wrote: > Perhaps some catch-all "I want defined behavior for things that C defines > even though I'm compiling in C++" flag would make sense

[PATCH] D27806: [clang-tidy] Add obvious-invalid-range

Quuxplusone added inline comments. Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:62 + auto CallsAlgorithm = hasDeclaration( + functionDecl(Names.size() > 0 ? hasAnyName(Names) : anything())); + Prazek wrote: > alexfh wrote: > > Does this check make

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

Quuxplusone added a comment. PVS-Studio implements tons of checks of this variety. See e.g. http://www.viva64.com/en/b/0299/#ID0E4KBM They don't have a catchy name for the category either, but perhaps "suspicious-" or "copypaste-" would do. I agree with Aaron that "thinko-" would be a little

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

Quuxplusone added inline comments. Comment at: include/memory:3933 +typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk; +__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT()); __hold.release(); EricWF wrote: >

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

Quuxplusone added a comment. I notice that the Standard implies that std::unique_ptr will work only with *object types* T, and yet the "object" wording is missing from shared_ptr. > A unique pointer is an object that owns another *object* and manages that > other *object* through a pointer.

[PATCH] D35941: Fix -Wshadow false positives with function-local classes.

Quuxplusone accepted this revision. Quuxplusone added a comment. > But if I'm overseeing reasons to issue a warning in this case, we could add > an analogue of `-Wshadow-uncaptured-local` for this case. WDYT? I still think "any warning" is a marginally better UX than "no warning" on the

[PATCH] D35863: Use the allocator's pointers for deallocation in `std::list`

Quuxplusone updated this revision to Diff 109678. Quuxplusone added a comment. I've updated https://reviews.llvm.org/D35863 to be actually correct AFAICT from my local testing; but I'm not sure what's the most appropriate way to get "fancy allocator" tests into libcxx's test suite. The way I

[PATCH] D35578: Add -fswitch-tables and -fno-switch-tables flags

Quuxplusone added a comment. If the goal is fine-grained control over the heuristics for compiling switch statements, perhaps one should enumerate all the possible ways to lower switch statements --- jump-tables, lookup-tables, if-trees, if-chains, (more?) --- and add a separate flag for each

[PATCH] D35824: [Sema] Implicitly apply the may_alias attribute to std::byte

Quuxplusone added inline comments. Comment at: test/CodeGenCXX/std-byte.cpp:7 +enum byte : unsigned char {}; +} + Would it be worth adding an explicit test that `::byte`, `::my::byte`, `::my::std::byte` are or are not handled in the same way?

[PATCH] D35863: Use the allocator's pointers for deallocation in `std::list`

Quuxplusone created this revision. `pointer_traits::to_pointer(r)` is not required to return a deallocatable pointer; indeed generally it *cannot* determine a deallocatable representation without help from the allocator. Therefore, we must not rely on representations derived from `to_pointer`

[PATCH] D34649: Remove addtional parameters in function std::next() and std::prev()

Quuxplusone added inline comments. Comment at: include/iterator:619 template inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 Might fix the spelling error in "BidirectionalIter" while you're here. https://reviews.llvm.org/D34649

[PATCH] D37958: [libc++] Correctly propagate user-defined lookup_classname().

Quuxplusone added inline comments. Comment at: libcxx/test/std/re/re.traits/lookup_classname_user_defined.pass.cpp:46 +// matches all characters (they are classified as alnum) +std::wstring re1 = L"([[:alnum:]]+)"; +std::regex_search(in, m, std::wregex(re1));

[PATCH] D37958: [libc++] Correctly propagate user-defined lookup_classname().

Quuxplusone added inline comments. Comment at: libcxx/test/std/re/re.traits/lookup_classname_user_defined.pass.cpp:46 +// matches all characters (they are classified as alnum) +std::wstring re1 = L"([[:alnum:]]+)"; +std::regex_search(in, m, std::wregex(re1));

[PATCH] D37677: [libc++] implement future synchronization using atomic_flag

Quuxplusone added a comment. TODO.txt says "future should use for synchronization." I would have interpreted this as meaning that the line unsigned __state_; should become std::atomic __state_; and appropriate loads and stores used so that `__is_ready()` can be checked without having

[PATCH] D37538: [libc++] Remove problematic ADL in container implementations.

Quuxplusone added inline comments. Comment at: include/memory:1349 is_same< -decltype(__has_construct_test(declval<_Alloc>(), - declval<_Pointer>(), - declval<_Args>()...)), +

[PATCH] D38599: Remove warnings for dynamic_cast fallback.

Quuxplusone added a comment. I'm also much out of my depth here, but I'm skeptical. You're changing the comments in the code from essentially saying "This workaround helps people with broken code" to essentially saying "This indispensable functionality helps people like me who use dlopen()."

[PATCH] D41316: [libcxx] Allow random_device to be built optionally

Quuxplusone added a comment. I keep seeing patches go by for other targets where they're just implementing `random_device` for their target. It doesn't *have* to be based on an actual /dev/urandom. You can see all the other options under #ifdefs in this file: getentropy, /dev/random,

[PATCH] D33776: [libcxx] LWG2221: No formatted output operator for nullptr

Quuxplusone added inline comments. Comment at: include/ostream:225 +basic_ostream& operator<<(nullptr_t) +{ return *this << (const void*)0; } + mclow.lists wrote: > lichray wrote: > > Oh, common, I persuaded the committee to allow you to print a `(null)`

[PATCH] D40144: Implement `std::launder`

Quuxplusone added inline comments. Comment at: test/std/language.support/support.dynamic/ptr.launder/launder.fail.cpp:26 +int *p = nullptr; +std::launder(p); // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}} +}

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

Quuxplusone added a comment. In https://reviews.llvm.org/D45766#1097797, @ksu.shadura wrote: > Thank you for the test example! I got your point, but I wanted to ask if it > should be like this for commutative operations? > In our case it is actually matrix, and subtraction of matrices is not

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. > Can you post the diagnostic exactly as it appears in the compiler output? I > am surprised that it would appear here. It should appear here only if the > standard library's implicit conversion from std::map<...>::iterator to > std::map<...>::const_iterator were

[PATCH] D46806: Remove unused code from __functional_base. NFC.

Quuxplusone created this revision. Quuxplusone added a reviewer: EricWF. Herald added a subscriber: cfe-commits. `__user_alloc_construct_impl` is used by , but this `__user_alloc_construct` is never used. Repository: rCXX libc++ https://reviews.llvm.org/D46806 Files:

[PATCH] D46807: Rename test_memory_resource.hpp -> test_experimental_memory_resource.hpp. NFC.

Quuxplusone created this revision. Quuxplusone added reviewers: EricWF, mclow.lists. Herald added subscribers: cfe-commits, christof. This makes room for a new "test_memory_resource.hpp", very similar to the old one, but testing the C++17 header instead of the experimental header. Repository:

[PATCH] D46806: Remove unused code from __functional_base. NFC.

Quuxplusone updated this revision to Diff 147677. Quuxplusone added a comment. @EricWF: would you mind landing these two drive-by fixes while you're at it? Repository: rCXX libc++ https://reviews.llvm.org/D46806 Files: include/__functional_base include/experimental/memory_resource

[PATCH] D47090: Implement C++17 .

Quuxplusone added inline comments. Comment at: include/__memory_resource_base:196 + typename __uses_alloc_ctor< + _T1, polymorphic_allocator&, _Args1... + >::type() >> (B) It's got different semantics around

[PATCH] D47109: LWG 2969 "polymorphic_allocator::construct() shouldn't pass resource()"

Quuxplusone created this revision. Quuxplusone added a reviewer: EricWF. Herald added a subscriber: cfe-commits. In the TS, `uses_allocator` construction for `pair` tried to use an allocator type of `memory_resource*`, which is incorrect because `memory_resource*` is not an allocator type. LWG

[PATCH] D47109: LWG 2969 "polymorphic_allocator::construct() shouldn't pass resource()"

Quuxplusone added a comment. In https://reviews.llvm.org/D47109#1105680, @EricWF wrote: > This LGTM. Let's land it and see if anybody complains. Sounds good to me. I don't have commit privs so could you land it for me? (Besides, I'm happy for you to get the blame if it *does* break someone's

[PATCH] D46806: Remove unused code from __functional_base. NFC.

Quuxplusone added inline comments. Comment at: include/experimental/memory_resource:99 // 8.5, memory.resource -class _LIBCPP_TEMPLATE_VIS memory_resource +class _LIBCPP_TYPE_VIS memory_resource { ...although maybe I don't understand the rules here. For

[PATCH] D47109: LWG 2969 "polymorphic_allocator::construct() shouldn't pass resource()"

Quuxplusone updated this revision to Diff 147676. Quuxplusone added a comment. Herald added a subscriber: christof. Fix two more tests hiding in test/libcxx/, and give -U999 context. Repository: rCXX libc++ https://reviews.llvm.org/D47109 Files: include/experimental/memory_resource

[PATCH] D47111: Implement monotonic_buffer_resource in

Quuxplusone created this revision. Quuxplusone added a reviewer: EricWF. Herald added a subscriber: cfe-commits. (https://reviews.llvm.org/D47090 included this in ``; at Eric's request, I've split this out into its own patch applied to the existing `` instead.) Repository: rCXX libc++

[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files

Quuxplusone added inline comments. Comment at: clang-tidy/tool/ClangTidyMain.cpp:188 +format to stderr. When this option is passed, +these per-TU profiles are instead stored as YAML.)"), + cl::value_desc("prefix"),

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if

[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types

Quuxplusone added inline comments. Comment at: include/charconv:89 +_LIBCPP_BEGIN_NAMESPACE_STD + +enum class _LIBCPP_ENUM_VIS chars_format lichray wrote: > mclow.lists wrote: > > lichray wrote: > > > EricWF wrote: > > > > We need to hide these names when

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added a comment. Just commenting to say that this LGTM and I have no further nitpicks. I have verified that I cannot detect any change in the behavior of `-Wpessimizing-move` or `-Wreturn-std-move` due to this change (and I //can// successfully detect the additional copy-elision

[PATCH] D47109: LWG 2969 "polymorphic_allocator::construct() shouldn't pass resource()"

Quuxplusone added a comment. > Sounds good to me. I don't have commit privs so could you land it for me? @EricWF ping! Repository: rCXX libc++ https://reviews.llvm.org/D47109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone created this revision. Quuxplusone added a reviewer: EricWF. Herald added subscribers: cfe-commits, JDevlieghere. https://github.com/cplusplus/draft/commit/6216651aada9bc2f9cefe90edbde4ea9e32251ab `new_delete_resource().allocate(n, a)` has basically three permissible results: -

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone updated this revision to Diff 148540. Quuxplusone added a comment. Oops. If we do get an underaligned result, let's not leak it! Repository: rCXX libc++ https://reviews.llvm.org/D47344 Files: src/experimental/memory_resource.cpp Index: src/experimental/memory_resource.cpp

[PATCH] D47090: Implement C++17 .

Quuxplusone abandoned this revision. Quuxplusone added a comment. I'm re-submitting this as a series of smaller patches that first bring `` up to date with C++17, and then copy it over to ``. In order, these smaller patches are: https://reviews.llvm.org/D46806 https://reviews.llvm.org/D47109

[PATCH] D46807: Rename test_memory_resource.hpp -> test_experimental_memory_resource.hpp. NFC.

Quuxplusone abandoned this revision. Quuxplusone added a comment. This is now part of https://reviews.llvm.org/D47360. Repository: rCXX libc++ https://reviews.llvm.org/D46807 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 148542. Quuxplusone retitled this revision from "Implement monotonic_buffer_resource in " to ": Implement monotonic_buffer_resource.". Quuxplusone added 1 blocking reviewer(s): EricWF. Quuxplusone added a comment. Fix one visibility macro. Repository:

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone created this revision. Quuxplusone added a reviewer: EricWF. Herald added a subscriber: cfe-commits. Split out from https://reviews.llvm.org/D47090. This patch is based on top of (depends on) https://reviews.llvm.org/D47111, but I'm posting it now for review. Repository: rCXX

[PATCH] D47090: Implement C++17 .

Quuxplusone added a comment. > I would prefer if we completed (according to > the current standard, not the LFTS spec), and then moved it. > Would you be willing to do that instead? Let me see if I understand. libc++'s `` differs from C++17 `` in at least these ways: (A) It's missing

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone added inline comments. Comment at: include/experimental/memory_resource:489 +memory_resource* __res_; +size_t __next_buffer_size_; +}; I've discovered that Boost.Container does not bother to preserve this state across calls to `release()`.

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone planned changes to this revision. Quuxplusone added inline comments. Comment at: src/experimental/memory_resource.cpp:240 +bool allocation_contains(const char *p) const { +// TODO: This part technically relies on undefined behavior. +return

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone added a comment. @EricWF ping; is this a reasonable solution to LWG 2843 on platforms without aligned new and delete? Repository: rCXX libc++ https://reviews.llvm.org/D47344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone added inline comments. Comment at: include/experimental/memory_resource:489 +memory_resource* __res_; +size_t __next_buffer_size_; +}; Quuxplusone wrote: > I've discovered that Boost.Container does not bother to preserve this state > across

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 151112. Quuxplusone added a comment. Herald added a subscriber: christof. - Shrink monotonic_buffer_resource, and make release() not "leak" allocation size. Repeatedly calling allocate() and release() in a loop should not blow up. I originally

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 151983. Quuxplusone added a comment. Minor cosmetic changes. Repository: rCXX libc++ https://reviews.llvm.org/D47111 Files: include/experimental/memory_resource src/experimental/memory_resource.cpp

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone updated this revision to Diff 151984. Quuxplusone added a comment. Herald added a subscriber: mgrang. Bugfix and shrink {un,}synchronized_pool_resource. The old implementation was severely broken in two ways: - It accidentally sometimes trusted the user's `bytes` and `align`

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone added inline comments. Comment at: include/experimental/memory_resource:429 +size_t __capacity_; +size_t __alignment_; +size_t __used_; EricWF wrote: > I can't imagine we'll need more than 1 byte to represent the alignment. Even assuming

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 148843. Quuxplusone added a comment. Add `override`; disintegrate the unit test; adopt some tests from Eric's https://reviews.llvm.org/D27402. Also fix one QOI issue discovered by Eric's tests: if the user passes an `initial_size` to the constructor,

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 148845. Quuxplusone added a comment. Refactor to remove unused fields from the header structs. Before this change, `sizeof(monotonic_buffer_resource) == 56` and the overhead per allocated chunk was `40` bytes. After this change,

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone updated this revision to Diff 149006. Quuxplusone added a comment. Rebase and update the diff. Repository: rCXX libc++ https://reviews.llvm.org/D47358 Files: include/experimental/memory_resource src/experimental/memory_resource.cpp

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone added inline comments. Comment at: test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp:11 +// UNSUPPORTED: c++98, c++03, c++11, c++14 +// UNSUPPORTED: apple-clang-7 + This test comes from Eric's D27402. The default-initialization

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone updated this revision to Diff 149611. Quuxplusone added a comment. - Split up the unit tests. - Refactor to shrink the memory layout of the resource object itself. Before this patch `sizeof(unsynchronized_pool_resource)==48`. After this patch `sizeof(unsynchronized_pool_resource) ==

[PATCH] D46806: Remove unused code from __functional_base. NFC.

Quuxplusone updated this revision to Diff 150067. Quuxplusone edited the summary of this revision. Quuxplusone added a comment. Also, `` doesn't need a full definition of `std::tuple`; just the forward declaration in `<__tuple>` will suffice. Repository: rCXX libc++

[PATCH] D47360: Implement as a copy of .

Quuxplusone planned changes to this revision. Quuxplusone added a comment. Once the dependencies (https://reviews.llvm.org/D47111 and https://reviews.llvm.org/D47358) are merged, I need to update the unit tests in this patch to reflect the unit tests that were committed. Right now, the unit

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone added a comment. Herald added a subscriber: ldionne. @EricWF ping! Repository: rCXX libc++ https://reviews.llvm.org/D47344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Quuxplusone added inline comments. Comment at: libcxx/include/memory:1479 +struct __has_construct_missing +: false_type +{ vsapsai wrote: > vsapsai wrote: > > vsapsai wrote: > > > erik.pilkington wrote: > > > > vsapsai wrote: > > > > > erik.pilkington wrote:

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 153881. Repository: rCXX libc++ https://reviews.llvm.org/D47111 Files: include/experimental/memory_resource src/experimental/memory_resource.cpp

[PATCH] D47358: : Implement {un, }synchronized_pool_resource.

Quuxplusone updated this revision to Diff 153882. Herald added subscribers: ldionne, christof. Repository: rCXX libc++ https://reviews.llvm.org/D47358 Files: include/experimental/memory_resource src/experimental/memory_resource.cpp

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. In https://reviews.llvm.org/D43322#1083075, @arthur.j.odwyer wrote: > Sorry, I responded via email but I guess Phabricator didn't pick it up for > some reason. > See below. And then Phab *still* didn't pick up the important part... Okay, I'll try pasting it

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

arthur.j.odwyer added a comment. Sorry, I responded via email but I guess Phabricator didn't pick it up for some reason. See below. Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone added inline comments. Comment at: src/experimental/memory_resource.cpp:48 +#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION +if (__size != 0 && !is_aligned_to(result, __align)) { +_VSTD::__libcpp_deallocate(result, __align); Quuxplusone

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone added inline comments. Comment at: src/experimental/memory_resource.cpp:29 +static bool is_aligned_to(void *ptr, size_t align) +{ +void *p2 = ptr; EricWF wrote: > Wait, can't this be written `reinterpret_cast(ptr) % align == 0`? Yes on sane

[PATCH] D48753: [libcxx] Use custom allocator's `construct` in C++03 when available.

Quuxplusone added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp:60 + + void construct(pointer p, const value_type& val) + { Per my comments on D48342, I think it would be fun to add a

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone marked 7 inline comments as done. Quuxplusone added a comment. > I'll take a pass at the tests tomorrow. Some pointers in general: > > - Tests should be named after the component they test, not how they're > testing it. > - All the tests for a single component should be in the same

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone updated this revision to Diff 154050. Quuxplusone marked 2 inline comments as done. Quuxplusone added a comment. Massive changes based on Eric's feedback. Moved constructors and destructors of `monotonic_buffer_resource` into the header so they can be completely inlined. Renamed

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Quuxplusone updated this revision to Diff 154041. Quuxplusone added a comment. Updated to no longer check "size != 0". Also rolled in some drive-by cosmetic refactoring that I'd done later in my branch: these functions aren't in a public header and don't need to be uglified. Repository: rCXX

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

Quuxplusone added inline comments. Comment at: libcxx/include/memory:1479 +struct __has_construct_missing +: false_type +{ vsapsai wrote: > Quuxplusone wrote: > > vsapsai wrote: > > > vsapsai wrote: > > > > vsapsai wrote: > > > > > erik.pilkington wrote: > >

[PATCH] D41740: [clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t

Quuxplusone added inline comments. Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:44 +} else if (Name == "int8_t") { +diag(Offender->getLocStart(), "streaming int8_t"); +break; I don't know clang-tidy style either, but might

[PATCH] D42248: Always allow "#pragma region".

Quuxplusone added a comment. FWIW, I would also like this patch, because it would mean that I could build with -Werror even when the project includes headers written by MSVC-using people. Given that we know what "#pragma region" does, it hardly deserves an "unknown pragma" diagnostic! So this

[PATCH] D42693: [libcxx] Handle invalid escaped characters in POSIX regex

Quuxplusone added inline comments. Comment at: include/regex:3465 +case '{': +case '}': +break; FWIW, I don't understand what's going on in this switch. Is it intentional that `'('` and `'|'` now take different paths here?

[PATCH] D42730: Add clang-tidy check for use of types/classes/functions from header which are deprecated and removed in C++17

Quuxplusone added inline comments. Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19 + +/// Check for several deprecated types and classes from header +/// aaron.ballman wrote: > alexfh wrote: > > alexfh wrote: > > > aaron.ballman wrote: > > > >

[PATCH] D41217: [Concepts] Concept Specialization Expressions

Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2404 +def err_concept_non_constant_constraint_expression : Error< + "concept specialization '%0' resulted in a non-constant expression '%1'.">; +def err_non_bool_atomic_constraint : Error<

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone created this revision. Quuxplusone added a project: clang. Herald added a subscriber: cfe-commits. This patch adds two new diagnostics, which are off by default: **-Wreturn-std-move** Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter,

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 134779. Quuxplusone added a comment. Add fixits. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. @rsmith and/or @rtrieu, please take another look? All my TODOs are done now: there are fixits, and the wording of the diagnostic changes if it's a "throw" instead of a "return", and the wording has been updated per Richard Smith's suggestions. I have one very

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 134999. Quuxplusone edited the summary of this revision. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 135005. Quuxplusone added a comment. Removed a redundant check for LValueReferenceType in the CWG1579 codepath. (In that branch, we know that standard C++ *did* perform the copy-to-move transformation, so obviously we can't have had an lvalue reference

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 135484. Quuxplusone added a reviewer: rsmith. Quuxplusone added a subscriber: Rakete. Quuxplusone added a comment. Eliminate a couple of `auto` per comment by @Rakete. Repository: rC Clang https://reviews.llvm.org/D43322 Files:

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 134514. Quuxplusone added a comment. Reword the diagnostic per rsmith's suggestions. Still TODO: - figure out how to detect whether this is a return-stmt or throw-expression - figure out how to generate the appropriate fixit Repository: rC Clang

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone marked an inline comment as done. Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:23 +def warn_return_std_move : Warning< + "adding 'std::move' here would select a better conversion sequence">, + InGroup, DefaultIgnore;

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. In https://reviews.llvm.org/D43322#1017190, @lebedev.ri wrote: > Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV IIUC, you're asking whether it would be possible to detect instances of return take(mysharedptr); and rewrite them into

[PATCH] D41316: [libcxx] Allow random_device to be built optionally

Quuxplusone added a comment. @weimingz: Since your platform supports `srand(0)`, is it possible to look at how your platform implements `srand(0)` and "inline" that implementation into `random_device`? That seems like it would be more in keeping with the other ifdefs in this file. I'm

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. @rtrieu please take a look? Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Quuxplusone added inline comments. Comment at: compiler-explorer-llvm-commit.sh:1 +# This is the commit of LLVM that we're currently based on. +git reset --hard 1fa19f68007cd126a04448093c171f40e556087e Rakete wrote: > What's this file? A mistake? Yeah, it's

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Quuxplusone updated this revision to Diff 158822. Quuxplusone marked 20 inline comments as done. Quuxplusone added a comment. Further removal of dead code based on @Rakete's feedback. Repository: rC Clang https://reviews.llvm.org/D50119 Files: docs/LanguageExtensions.rst

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Quuxplusone marked an inline comment as done. Quuxplusone added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:6091 + for (auto *F : Record->fields()) { +if (F->isMutable()) { Rakete wrote: > Can you move this in `ActOnFields`? That way we

[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

Quuxplusone added inline comments. Comment at: include/vector:318 +} +} + Marshall writes: > Instead, we should be writing simple loops that the compiler can optimize > into a call to memcpy if it chooses. Having calls to memcpy in the code paths > makes

[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

Quuxplusone added inline comments. Comment at: include/vector:318 +} +} + mclow.lists wrote: > Quuxplusone wrote: > > Marshall writes: > > > Instead, we should be writing simple loops that the compiler can optimize > > > into a call to memcpy if it chooses.

[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

Quuxplusone added a comment. @mclow.lists: Well, anyway, is this pure refactoring acceptable, or would you prefer to keep these helpers inside `allocator_traits` until a better solution to constexpr-memcpy can be invented? Repository: rCXX libc++ https://reviews.llvm.org/D49317

[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

Quuxplusone added a comment. In https://reviews.llvm.org/D49317#1180852, @ldionne wrote: > After thinking about this for some more, I'm not sure this patch is worth > doing in its current form. The minimal patch for allowing specializations of > `allocator_traits` would be: > > 1. move the

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Quuxplusone created this revision. Quuxplusone added a reviewer: rsmith. Quuxplusone added a project: clang. Herald added a subscriber: cfe-commits. This is the compiler half of C++ proposal 1144 "Object relocation in terms of move plus destroy," as seen on https://godbolt.org/g/zUUAVW and

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:6187 +Record->dropAttr(); + } else if (Record->needsImplicitMoveConstructor() && + Record->defaultedMoveConstructorIsDeleted()) { Rakete wrote: > Rakete

  1   2   3   4   5   6   7   >