[PATCH] D50421: [libcxx] [test] Remove assertion that includes and .
Quuxplusone added inline comments.
Comment at: test/std/utilities/template.bitset/includes.pass.cpp:11
+// test that includes , and
#include
template void test_typedef() {}
grammar nit: comma
https://reviews.llvm.org/D50421
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41217: [Concepts] Concept Specialization Expressions
Quuxplusone added inline comments.
Comment at: include/clang/Sema/Sema.h:7134
+ // We are substituting template arguments into a constraint expression.
+ ConstraintSubstitution
} Kind;
Missing a trailing comma here.
Comment at: test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp:121
+template
+concept C7 = sizeof(T) == 1 || sizeof(typename T3::type) == 1; //
expected-note{{while substituting template arguments into constraint expression
here}} expected-note{{in instantiation of template class 'T3' requested
here}}
+
Nit: You could use `// expected-note@-1{{...}}`, `// expected-note@-2{{...}}`
to make lines like this more readable.
Comment at: test/Parser/cxx-concept-declaration.cpp:48
+bool a = C16;
+bool b = C17;
Should you static-assert the expected results?
```
static_assert(!C12);
static_assert(C13);
static_assert(C14);
static_assert(C15);
static_assert(C16);
static_assert(C16);
static_assert(C17);
static_assert(!C17);
```
Repository:
rC Clang
https://reviews.llvm.org/D41217
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47111: : Implement monotonic_buffer_resource.
Quuxplusone added a comment. Long-delayed ping! Repository: rCXX libc++ https://reviews.llvm.org/D47111 ___ 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 updated this revision to Diff 160642.
Quuxplusone marked an inline comment as done.
Quuxplusone added a comment.
Rebased, and ping!
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/AST/Type.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,510 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is
+// either not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) = default; };
+// expected-error@-1{{cannot be applied to struct 'D6' because it is not move-constructible}}
+
+template
+struct [[trivially_relocatable]] DT1 : private T { }; // ok
+
+struct D7 {
+
[PATCH] D50805: Don't warn on returning the address of a label
Quuxplusone added a comment.
> I think that, ultimately, this warning isn't worth the code complexity in
> clang to support it.
The code complexity is //at most// 11 lines (the 11 lines removed by this
patch), which really isn't much. But there's an interface-complexity argument
to be made, for sure. If you added a new option `-Wret-addr-label` as
suggested above (for a total patch of +2 lines), then is it accurate to say:
- if `-Wret-addr-label` was enabled by default, we know of at least one
codebase that would pass `-Wno-ret-addr-label` to their build
- if `-Wret-addr-label` was disabled by default, we don't know of any codebases
that would voluntarily enable it
And if nobody would enable it voluntarily... might as well eliminate it, right?
Comment at: clang/test/Analysis/stack-addr-ps.cpp:81
label:
-void *const &x = &&label; // expected-note {{binding reference variable
'x' here}}
-return x; // expected-warning {{returning address of label, which is
local}}
+void *const &x = &&label;
+return x;
Is it just me, or is this test case relying on lifetime extension for no good
reason? Why is this not `void *const x = &&label;`?
https://reviews.llvm.org/D50805
___
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 updated this revision to Diff 161190.
Quuxplusone added a comment.
A type with no move-constructor, and some non-defaulted copy-constructors (even
if it *also* has some defaulted copy-constructors) should not be considered
naturally trivially relocatable. The paper already said this, but the Clang
patch was misimplemented.
Thanks to @Rakete for either finding or inspiring this fix. :)
I suspect there may be some way of doing this without the extra bitflag, but I
didn't immediately see how.
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/AST/Type.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,518 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is
+// either not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5'
[PATCH] D41284: [Concepts] Associated constraints infrastructure.
Quuxplusone added inline comments. Comment at: test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp:10 + +template requires bool(U()) +int B::A = int(U()); For my own edification, could you explain whether, given #define BOOL bool using typedef_for_bool = bool; you'd expect to diagnose a redeclaration of `B::A` with associated constraint requires bool( U() ) // extra whitespace or requires BOOL(U()) // different spelling of `bool` or requires typedef_for_bool(U()) // different spelling of `bool` ? My naive reading of N4762 temp.constr.atomic/2 says that none of these constraints (on line 10) would be "identical" to the constraint on line 6... but then I don't understand what's the salient difference between line 10 (which apparently gives no error) and line 22 (which apparently gives an error). Repository: rC Clang https://reviews.llvm.org/D41284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41284: [Concepts] Associated constraints infrastructure.
Quuxplusone added inline comments. Comment at: test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp:10 + +template requires bool(U()) +int B::A = int(U()); saar.raz wrote: > Quuxplusone wrote: > > For my own edification, could you explain whether, given > > > > #define BOOL bool > > using typedef_for_bool = bool; > > > > you'd expect to diagnose a redeclaration of `B::A` with associated > > constraint > > > > requires bool( U() ) // extra whitespace > > > > or > > > > requires BOOL(U()) // different spelling of `bool` > > > > or > > > > requires typedef_for_bool(U()) // different spelling of `bool` > > > > ? My naive reading of N4762 temp.constr.atomic/2 says that none of these > > constraints (on line 10) would be "identical" to the constraint on line > > 6... but then I don't understand what's the salient difference between line > > 10 (which apparently gives no error) and line 22 (which apparently gives an > > error). > Line 22 has a not (!) operator in front of the bool(), I guess you missed > that? I saw the `!`... but I don't understand how the compiler "knows" that `!bool(U())` is "different" from `bool(T())` in a way that doesn't equally apply to `bool(U())`. Or suppose the constraint on line 10 was `requires bool(U())==true`... would that give a diagnostic? Repository: rC Clang https://reviews.llvm.org/D41284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41284: [Concepts] Associated constraints infrastructure.
Quuxplusone added inline comments. Comment at: test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/var-template-decl.cpp:10 + +template requires bool(U()) +int B::A = int(U()); saar.raz wrote: > saar.raz wrote: > > Rakete wrote: > > > Quuxplusone wrote: > > > > saar.raz wrote: > > > > > Quuxplusone wrote: > > > > > > For my own edification, could you explain whether, given > > > > > > > > > > > > #define BOOL bool > > > > > > using typedef_for_bool = bool; > > > > > > > > > > > > you'd expect to diagnose a redeclaration of `B::A` with associated > > > > > > constraint > > > > > > > > > > > > requires bool( U() ) // extra whitespace > > > > > > > > > > > > or > > > > > > > > > > > > requires BOOL(U()) // different spelling of `bool` > > > > > > > > > > > > or > > > > > > > > > > > > requires typedef_for_bool(U()) // different spelling of `bool` > > > > > > > > > > > > ? My naive reading of N4762 temp.constr.atomic/2 says that none of > > > > > > these constraints (on line 10) would be "identical" to the > > > > > > constraint on line 6... but then I don't understand what's the > > > > > > salient difference between line 10 (which apparently gives no > > > > > > error) and line 22 (which apparently gives an error). > > > > > Line 22 has a not (!) operator in front of the bool(), I guess you > > > > > missed that? > > > > I saw the `!`... but I don't understand how the compiler "knows" that > > > > `!bool(U())` is "different" from `bool(T())` in a way that doesn't > > > > equally apply to `bool(U())`. > > > > > > > > Or suppose the constraint on line 10 was `requires bool(U())==true`... > > > > would that give a diagnostic? > > > `bool(T())` and `bool(U())` are identical because they have the same > > > parameter mappings. > > > > > > The "identical" requirement applies to the actual grammar composition of > > > the expression, so `bool(T())` would be different to `bool(T()) == true`. > > > > > > At least that's how I understand it. > > OK, I can see where the confusion is coming from. > > > > The way it works (in clang, at least) - is that the compiler pays no > > attention to the name of a template parameter for any purpose other than > > actually finding it in the first place - once it is found, it is 'stored' > > simply as bool(()) where the first 0 is the depth > > of the template parameter list of the parameter in question (in case of a > > template within a template) and the second 0 is the index of the template > > parameter within that list. > > > > I believe this treatment stems from [temp.over.link]p6 "When determining > > whether types or qualified-concept-names are equivalent, the rules above > > are used to compare expressions involving template parameters" > Correction - p5 describes this better (see also the example in p4) Okay, I can see how this matches the apparent intent of http://eel.is/c++draft/temp.over.link#5 . I guess a strict reading of that passage would mean that my `BOOL` and `typedef_for_bool` versions should give diagnostics, and so should ``` #define V(x) U template requires bool(V(x) ()) ``` but no diagnostic is expected for ``` #define V U template requires bool(V ()) ``` Anyway, sounds like this rabbit hole is out-of-scope for this review, anyway, so I'll be quiet now. :) Repository: rC Clang https://reviews.llvm.org/D41284 ___ 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 updated this revision to Diff 161866.
Quuxplusone added a comment.
Update to match the wording in D1144R0 draft revision 13. There's no longer any
need to fail when we see a mutable member; on the other hand, we *always* need
to perform the special member lookup to find out if our class is
move-constructible and destructible.
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/AST/Type.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,592 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is
+// either not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) =
[PATCH] D51135: Rename has{MutableFields, VolatileMember} to has{Mutable, Volatile}Subobject. NFC.
Quuxplusone created this revision.
Herald added a subscriber: cfe-commits.
Rename `HasMutableFields` to `HasMutableSubobject` for accuracy.
This bitflag is set whenever the record type has a mutable subobject,
regardless of whether that subobject is a direct member, or a member
of some base class, or a member of a member, etc. etc.
It does not need to be a field of the record type itself.
Rename `HasVolatileMember` to `HasVolatileSubobject` for accuracy.
This bitflag is set whenever the record type has a volatile subobject,
regardless of whether that subobject is a direct member, or a member
of some base class, or a member of a member, etc. etc.
It does not need to be a member of the record type itself.
Generally in C++ if we have
struct A { mutable int m; };
struct B { A a; };
we say that `a` is a "member" or "field" of `B`; `m` is a "subobject" of `B`;
but `m` is neither a "member" nor a "field" of `B`.
The current naming of these accessors confused me for a little while during the
development of a different patch, so I thought I'd submit a diff renaming them
after what they actually do, and see if anyone thought it was a good idea. (I
don't have commit privs.)
Repository:
rC Clang
https://reviews.llvm.org/D51135
Files:
include/clang/AST/Decl.h
include/clang/AST/DeclBase.h
include/clang/AST/DeclCXX.h
lib/AST/ASTDumper.cpp
lib/AST/ASTImporter.cpp
lib/AST/Decl.cpp
lib/AST/DeclCXX.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGAtomic.cpp
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CodeGenFunction.h
lib/CodeGen/CodeGenModule.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaOpenMP.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
lib/Serialization/ASTWriterDecl.cpp
lib/StaticAnalyzer/Core/CallEvent.cpp
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -675,7 +675,7 @@
T = T->getPointeeType();
const CXXRecordDecl *ParentRecord = T->getAsCXXRecordDecl();
assert(ParentRecord);
-if (ParentRecord->hasMutableFields())
+if (ParentRecord->hasMutableSubobject())
return;
// Preserve CXXThis.
const MemRegion *ThisRegion = ThisVal.getAsRegion();
Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -468,7 +468,7 @@
Record.push_back(D->hasFlexibleArrayMember());
Record.push_back(D->isAnonymousStructOrUnion());
Record.push_back(D->hasObjectMember());
- Record.push_back(D->hasVolatileMember());
+ Record.push_back(D->hasVolatileSubobject());
Record.push_back(D->isNonTrivialToPrimitiveDefaultInitialize());
Record.push_back(D->isNonTrivialToPrimitiveCopy());
Record.push_back(D->isNonTrivialToPrimitiveDestroy());
@@ -1920,7 +1920,7 @@
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // FlexibleArrayMember
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // AnonymousStructUnion
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // hasObjectMember
- Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // hasVolatileMember
+ Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // hasVolatileSubobject
// isNonTrivialToPrimitiveDefaultInitialize
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1));
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -6025,7 +6025,7 @@
Record->push_back(Data.HasPrivateFields);
Record->push_back(Data.HasProtectedFields);
Record->push_back(Data.HasPublicFields);
- Record->push_back(Data.HasMutableFields);
+ Record->push_back(Data.HasMutableSubobject);
Record->push_back(Data.HasVariantMembers);
Record->push_back(Data.HasOnlyCMembers);
Record->push_back(Data.HasInClassInitializer);
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -787,7 +787,7 @@
RD->setHasFlexibleArrayMember(Record.readInt());
RD->setAnonymousStructOrUnion(Record.readInt());
RD->setHasObjectMember(Record.readInt());
- RD->setHasVolatileMember(Record.readInt());
+ RD->setHasVolatileSubobject(Record.readInt());
RD->setNonTrivialToPrimitiveDefaultInitialize(Record.readInt());
RD->setNonTrivialToPrimitiveCopy(Record.readInt());
RD->setNonTrivialToPrimitiveDestroy(Record.readInt());
@@ -1651,7 +1651,7 @@
Data.HasPrivateFields = Record.readInt();
Data.HasProtectedFields = Record.readInt();
Data.HasPublicFields = Record.readInt();
- Data.HasMutableFields = Record.readInt();
+ Data.HasMutableSubobject = Record.rea
[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 implemented as a conversion operator > instead of as a converting constructor. I have verified that both libstdc++ > trunk and libc++ trunk implement it "correctly" as a converting constructor, > and I have verified on Wandbox/Godbolt that no warning is emitted on your > sample code when using either of those standard libraries. > > Is it possible that you are using a standard library that is neither libc++ > or libstdc++? > > Is it possible that that standard library implements the > iterator-to-const_iterator conversion as a conversion operator instead of a > converting constructor? @thakis ping — did you ever resolve this issue to your satisfaction? 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] 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:
include/__functional_base
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args
&&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args &&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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:
rCXX libc++
https://reviews.llvm.org/D46807
Files:
test/libcxx/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair.pass.cpp
test/libcxx/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/db_deallocate.pass.cpp
test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.ctor/default.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.ctor/memory_resource_convert.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.eq/equal.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.eq/not_equal.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/allocate.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_rvalue.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_values.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/deallocate.pass.cpp
test/std/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.ctor/alloc_copy.pass.cpp
test/std/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.ctor/alloc_move.pass.cpp
test/std/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.ctor/default.pass.cpp
test/std/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/do_allocate_and_deallocate.pass.cpp
test/std/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/do_is_equal.pass.cpp
test/std/experimental/memory/memory.resource.global/default_resource.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.eq/equal.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.eq/not_equal.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.public/allocate.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.public/deallocate.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.public/dtor.pass.cpp
test/std/experimental/memory/memory.resource/memory.resource.public/is_equal.pass.cpp
test/support/test_experimental_memory_resource.hpp
test/support/test_memory_resource.hpp
test/support/uses_alloc_types.hpp
Index: test/support/uses_alloc_types.hpp
===
--- test/support/uses_alloc_types.hpp
+++ test/support/uses_alloc_types.hpp
@@ -143,7 +143,7 @@
// polymorphic allocators. However we don't want to include
// in this header. Therefore in order
// to inject this behavior later we use a trait.
-// See test_memory_resource.hpp for more info.
+// See test_experimental_memory_resource.hpp for more info.
template
struct TransformErasedTypeAlloc {
using type = Alloc;
Index: test/support/test_memory_resource.hpp
===
--- test/support/test_memory_resource.hpp
+++ /dev/null
@@ -1,171 +0,0 @@
-//===--===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is dual licensed under the MIT and the University of Illinois Open
-// Source Licenses. See LICENSE.TXT for details.
-//
-//===--===//
-
-#ifndef SUPPORT_TEST_MEMORY_RESOURCE_HPP
-#define SUPPORT_TEST_MEMORY_RESOURCE_HPP
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include "test_macros.h"
-#include "controlled_allocators.hpp"
-#include "uses_alloc_types.hpp"
-
-// FIXME: This is a hack to allow uses_allocator_types.hpp to work with
-// erased_type. However we can't define that behavior directly in the header
-// because it can't include
-template <>
-struct TransformErasedTypeAlloc {
- using type = std:
[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 > commutative operation.. Mathematical commutativity has nothing to do with this diagnostic. The diagnostic is complaining that `m1 -= m1` produces a trivial effect (in this case `m1.clear()`) via a syntax that might indicate that the programmer made a typo (i.e. the compiler suspects that you meant `m -= m1` or something). From the function name `stress_...`, I infer that this is a unit-test. This warning is known to give "false positives" in unit-test code, where one often actually wants to achieve trivial effects through complicated code. The current recommendation there, AFAIK, is either to write something like m1 -= static_cast(m1); // hide the self-subtraction from the compiler or else to add `-Wno-self-assign-overloaded` to your compiler flags. (libc++'s test suite already passes `-Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-conversion -Wno-unused-local-typedef -Wno-#warnings`, for example.) (That said, I continue to think that this diagnostic produces more noise than signal, and wish it weren't in `-Wall`...) Repository: rL LLVM https://reviews.llvm.org/D45766 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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"),
Drive-by nit: Each other help string ends with a newline (`)"),` on a line by
itself). This one presumably should too.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 `monotonic_buffer_resource` and
`{un,}synchronized_pool_resource`
(B) It's got different semantics around uses-allocator construction because of
https://wg21.link/lwg2969
(C) It's got a different header name
This patch is basically proposing to fix things in the order C-A-B (and fix C
by making copies of everything); you're proposing to fix things in the order
A-B-C (and fix C by moving everything).
I have no objection to fixing A in `` if people
think that'd be useful. I didn't do that in this patch simply because I'd
observed other `` headers already getting replaced with
`#error` messages, and it seemed like any further work on the
`` headers would have been wasted.
I don't know who's using `` today, but in theory
I worry that fixing B in `` (before doing C)
might actually break someone's code.
Philosophically as I understand it LWG2969 was a patch against C++17, not a
patch against the TS. If LWG2969 (and incidentally I asked lwgchair to open a
new issue for https://github.com/Quuxplusone/libcxx/commit/54e1a59a ) are
actually supposed to be patches against *both* C++17 *and* the TS, then cool,
yeah, we should fix B in ``. Thoughts?
Repository:
rCXX libc++
https://reviews.llvm.org/D47090
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 uses-allocator construction because
>> of https://wg21.link/lwg2969
> Issue resolutions should probably be applied to the experimental versions as
> well.
Okay, I can roll with that. I'll create a new patch that modifies
``'s uses-allocator parts per LWG2969 (and my
followup DR), without any other diffs.
Should `` continue caring about
`std::experimental::erased_type`, which was in LFTS and LFTSv2 but did not make
it into C++17? My kneejerk reaction is "yes". (And at the end of this
process, when we copy `` into
``, should `` care about `erased_type`? My
kneejerk reaction is "no".)
Comment at: src/memory_resource.cpp:62
+
+namespace {
+
EricWF wrote:
> We certainly don't want a different definition of the global resources in
> each TU. See below.
@EricWF: I think all of your comments in this file are the result of misreading
"src/memory_resource.cpp" as "include/memory_resource". Or else I *totally*
don't understand the current directory organization and you're going to have to
instruct me. :)
Repository:
rCXX libc++
https://reviews.llvm.org/D47090
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 2969 fixed it to use `polymorphic_allocator` as the
allocator type instead.
https://wg21.link/lwg2969
(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++
https://reviews.llvm.org/D47109
Files:
include/experimental/memory_resource
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_rvalue.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_values.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
test/support/test_memory_resource.hpp
Index: test/support/test_memory_resource.hpp
===
--- test/support/test_memory_resource.hpp
+++ test/support/test_memory_resource.hpp
@@ -28,7 +28,7 @@
// because it can't include
template <>
struct TransformErasedTypeAlloc {
- using type = std::experimental::pmr::memory_resource*;
+ using type = std::experimental::pmr::polymorphic_allocator;
};
template
Index: test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
===
--- test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
+++ test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
@@ -126,6 +126,39 @@
}
}
+template
+void test_pmr_not_uses_alloc(Args&&... args)
+{
+TestResource R(12435);
+ex::memory_resource* M = &R;
+{
+// NotUsesAllocator provides valid signatures for each uses-allocator
+// construction but does not supply the required allocator_type typedef.
+// Test that we can call these constructors manually without
+// polymorphic_allocator interfering.
+using T = NotUsesAllocator;
+assert(doTestUsesAllocV0(std::forward(args)...));
+assert((doTestUsesAllocV1(M, std::forward(args)...)));
+assert((doTestUsesAllocV2(M, std::forward(args)...)));
+}
+{
+// Test T(std::allocator_arg_t, Alloc const&, Args...) construction
+using T = UsesAllocatorV1;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+{
+// Test T(Args..., Alloc const&) construction
+using T = UsesAllocatorV2;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+{
+// Test that T(std::allocator_arg_t, Alloc const&, Args...) construction
+// is preferred when T(Args..., Alloc const&) is also available.
+using T = UsesAllocatorV3;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+}
+
// Test that polymorphic_allocator does not prevent us from manually
// doing non-pmr uses-allocator construction.
template
@@ -167,16 +200,16 @@
const int cvalue = 43;
{
test_pmr_uses_alloc();
-test_pmr_uses_alloc();
+test_pmr_not_uses_alloc();
test_pmr_uses_alloc();
test_pmr_uses_alloc(value);
-test_pmr_uses_alloc(value);
+test_pmr_not_uses_alloc(value);
test_pmr_uses_alloc(value);
test_pmr_uses_alloc(cvalue);
-test_pmr_uses_alloc(cvalue);
+test_pmr_not_uses_alloc(cvalue);
test_pmr_uses_alloc(cvalue);
test_pmr_uses_alloc(cvalue, std::move(value));
-test_pmr_uses_alloc(cvalue, std::move(value));
+test_pmr_not_uses_alloc(cvalue, std::move(value));
test_pmr_uses_alloc(cvalue, std::move(value));
}
{
Index: test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
@@ -0,0 +1,141 @@
+//===
[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
test/libcxx/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_rvalue.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_values.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
test/support/test_memory_resource.hpp
Index: test/support/test_memory_resource.hpp
===
--- test/support/test_memory_resource.hpp
+++ test/support/test_memory_resource.hpp
@@ -28,7 +28,7 @@
// because it can't include
template <>
struct TransformErasedTypeAlloc {
- using type = std::experimental::pmr::memory_resource*;
+ using type = std::experimental::pmr::polymorphic_allocator;
};
template
Index: test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
===
--- test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
+++ test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_types.pass.cpp
@@ -126,6 +126,39 @@
}
}
+template
+void test_pmr_not_uses_alloc(Args&&... args)
+{
+TestResource R(12435);
+ex::memory_resource* M = &R;
+{
+// NotUsesAllocator provides valid signatures for each uses-allocator
+// construction but does not supply the required allocator_type typedef.
+// Test that we can call these constructors manually without
+// polymorphic_allocator interfering.
+using T = NotUsesAllocator;
+assert(doTestUsesAllocV0(std::forward(args)...));
+assert((doTestUsesAllocV1(M, std::forward(args)...)));
+assert((doTestUsesAllocV2(M, std::forward(args)...)));
+}
+{
+// Test T(std::allocator_arg_t, Alloc const&, Args...) construction
+using T = UsesAllocatorV1;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+{
+// Test T(Args..., Alloc const&) construction
+using T = UsesAllocatorV2;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+{
+// Test that T(std::allocator_arg_t, Alloc const&, Args...) construction
+// is preferred when T(Args..., Alloc const&) is also available.
+using T = UsesAllocatorV3;
+assert((doTest(UA_None, std::forward(args)...)));
+}
+}
+
// Test that polymorphic_allocator does not prevent us from manually
// doing non-pmr uses-allocator construction.
template
@@ -167,16 +200,16 @@
const int cvalue = 43;
{
test_pmr_uses_alloc();
-test_pmr_uses_alloc();
+test_pmr_not_uses_alloc();
test_pmr_uses_alloc();
test_pmr_uses_alloc(value);
-test_pmr_uses_alloc(value);
+test_pmr_not_uses_alloc(value);
test_pmr_uses_alloc(value);
test_pmr_uses_alloc(cvalue);
-test_pmr_uses_alloc(cvalue);
+test_pmr_not_uses_alloc(cvalue);
test_pmr_uses_alloc(cvalue);
test_pmr_uses_alloc(cvalue, std::move(value));
-test_pmr_uses_alloc(cvalue, std::move(value));
+test_pmr_not_uses_alloc(cvalue, std::move(value));
test_pmr_uses_alloc(cvalue, std::move(value));
}
{
Index: test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_piecewise_pair_evil.pass.cpp
@@ -0,0 +1,141 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--
[PATCH] 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 code, as I suspect it might. ;)) Repository: rCXX libc++ https://reviews.llvm.org/D47109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
Index: include/experimental/memory_resource
===
--- include/experimental/memory_resource
+++ include/experimental/memory_resource
@@ -96,7 +96,7 @@
}
// 8.5, memory.resource
-class _LIBCPP_TEMPLATE_VIS memory_resource
+class _LIBCPP_TYPE_VIS memory_resource
{
static const size_t __max_align = alignof(max_align_t);
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args
&&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
Index: include/experimental/memory_resource
===
--- include/experimental/memory_resource
+++ include/experimental/memory_resource
@@ -96,7 +96,7 @@
}
// 8.5, memory.resource
-class _LIBCPP_TEMPLATE_VIS memory_resource
+class _LIBCPP_TYPE_VIS memory_resource
{
static const size_t __max_align = alignof(max_align_t);
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args &&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 example, I see
`allocator_arg_t` and `monostate` are both marked `_LIBCPP_TEMPLATE_VIS` as
well, even though they're not templates.
Repository:
rCXX libc++
https://reviews.llvm.org/D46806
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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++
https://reviews.llvm.org/D47111
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
@@ -0,0 +1,408 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class monotonic_buffer_resource
+
+#include
+#include
+#include
+#include
+
+#include "count_new.hpp"
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+struct repointable_resource : public std::experimental::pmr::memory_resource
+{
+std::experimental::pmr::memory_resource *which;
+
+explicit repointable_resource(std::experimental::pmr::memory_resource *res) : which(res) {}
+
+protected:
+virtual void *do_allocate(size_t size, size_t align)
+{ return which->allocate(size, align); }
+
+virtual void do_deallocate(void *p, size_t size, size_t align)
+{ return which->deallocate(p, size, align); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &rhs) const noexcept
+{ return which->is_equal(rhs); }
+};
+
+void test_construction_with_default_resource()
+{
+std::experimental::pmr::memory_resource *expected = std::experimental::pmr::null_memory_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+char buffer[16];
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2(16);
+std::experimental::pmr::monotonic_buffer_resource r3(buffer, sizeof buffer);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+assert(r3.upstream_resource() == expected);
+}
+
+expected = std::experimental::pmr::new_delete_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+char buffer[16];
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2(16);
+std::experimental::pmr::monotonic_buffer_resource r3(buffer, sizeof buffer);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+assert(r3.upstream_resource() == expected);
+}
+}
+
+void test_construction_without_buffer()
+{
+// Constructing a monotonic_buffer_resource should not cause allocations
+// by itself; the resource should wait to allocate until an allocation is
+// requested.
+
+globalMemCounter.reset();
+std::experimental::pmr::set_default_resource(std::experimental::pmr::new_delete_resource());
+
+std::experimental::pmr::monotonic_buffer_resource r1;
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::monotonic_buffer_resource r2(1024);
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::monotonic_buffer_resource r3(1024, std::experimental::pmr::new_delete_resource());
+assert(globalMemCounter.checkNewCalledEq(0));
+}
+
+void test_equality()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::monotonic_buffer_resource mono1;
+std::experimental::pmr::memory_resource & r1 = mono1;
+assert_on_compare c;
+std::experimental::pmr::memory_resource & r2 = c;
+
[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 `_LIBCPP_STD_VER < 17`, since we're > > > > not allowed to introduce new names into namespace `std` in older > > > > dialects. > > > But this header is backported to C++11, so I intended to not to guard it. > > > But this header is backported to C++11, so I intended to not to guard it. > > > > In general, we don't provide new features for old language versions. > > > > The only time we've ever done that is for `string_view`, and I'm **still** > > not sure I did the right thing there. > We need to decide on this... From my point of view this header will be widely > used by formatting and logging libraries, and it doesn't add much to the > community by enforcing C++17 here, especially when the interface we specified > are very simple and not using any features beyond C++11. This question is also relevant to my interests, in re ``. Repository: rCXX libc++ https://reviews.llvm.org/D41458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 (!FD->getReturnType()->isScalarType())
computeNRVO(Body, getCurFunction());
What is the purpose of this change?
If it's "no functional change" it should be done separately IMHO; if it is
supposed to change codegen, then it needs some codegen tests. (From looking at
the code: maybe this affects codegen on functions that return member function
pointers by value?)
Comment at: test/CodeGenCXX/nrvo.cpp:139
}
- // FIXME: we should NRVO this variable too.
- X x;
+ // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
return x;
You've changed this function from testing one thing with a FIXME, to testing a
completely different thing. Could you add your new code as a new function, and
leave the old FIXME alone until it's fixed?
Alternatively, if your patch actually fixes the FIXME, could you just replace
the FIXME comment with a CHECK and leave the rest of this function alone?
Comment at: test/CodeGenCXX/nrvo.cpp:254
+ T t;
+ return t;
+}
Just for my own curiosity: this new test case is surely unaffected by this
patch, right?
Repository:
rC Clang
https://reviews.llvm.org/D47067
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 (!FD->getReturnType()->isScalarType())
computeNRVO(Body, getCurFunction());
rsmith wrote:
> tzik wrote:
> > Quuxplusone wrote:
> > > What is the purpose of this change?
> > > If it's "no functional change" it should be done separately IMHO; if it
> > > is supposed to change codegen, then it needs some codegen tests. (From
> > > looking at the code: maybe this affects codegen on functions that return
> > > member function pointers by value?)
> > I think the previous implementation was incorrect.
> > Though computeNRVO clears ReturnStmt::NRVOCandidate when the corresponding
> > variable is not NRVO variable, CGStmt checks both of
> > ReturnStmt::NRVOCandidate and VarDecl::NRVOVariable anyway.
> > So, computeNRVO took no effect in the previous code, and the absence of
> > computeNRVO here on function templates did not matter.
> > Note that there was no chance to call computeNRVO on function template
> > elsewhere too.
> >
> > OTOH in the new implementation, computeNRVO is necessary, and its absence
> > on function templates matters.
> >
> > We can remove computeNRVO here separately as a NFC patch and readd the new
> > impl to the same place, but it's probably less readable, IMO.
> What happens if we can't tell whether we have an NRVO candidate or not until
> instantiation:
>
> ```
> template X f() {
> T v;
> return v; // is an NRVO candidate if T is X, otherwise is not
> }
> ```
>
> (I think this is not hard to handle: the dependent construct here can only
> affect whether you get NRVO at all, not which variable you perform NRVO on,
> but I want to check that it is handled properly.)
Ah, so if I understand correctly — which I probably don't —...
the parts of this diff related to `isRecordType()` make NRVO more reliable on
template functions? Because the existing code has lots of checks for
`isRecordType()` which means that they don't run for dependent (as-yet-unknown)
types, even if those types are going to turn out to be record types
occasionally?
I see that line 12838 runs `computeNRVO` unconditionally for *all*
ObjCMethodDecls, //even// ones with scalar return types. So maybe running
`computeNRVO` unconditionally right here would also be correct?
Datapoint: I made a patch to do nothing but replace this condition with `if
(Body)` and to remove the similar condition guarding `computeNRVO` in
`Sema::ActOnBlockStmtExpr`. It passed the Clang test suite with flying colors.
I don't know if this means that the test suite is missing some tests, or if it
means that the conditions are unnecessary.
IMHO if you're changing this anyway, it would be nice to put the remaining
condition/optimization `if (getReturnType().isScalarType()) return;` inside
`computeNRVO` itself, instead of repeating that condition at every call site.
Repository:
rC Clang
https://reviews.llvm.org/D47067
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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:
- Return an appropriately sized and aligned block.
- Throw bad_alloc.
- If `n == 0`, return an unspecified result.
Before this patch, libc++'s `new_delete_resource` would do a fourth and
impermissible thing, which was to return an appropriately sized but
inappropriately under-aligned block. This is now fixed.
(This came up while I was stress-testing `unsynchronized_pool_resource` on my
MacBook. If we can't trust the default resource to return appropriately aligned
blocks, pretty much everything breaks. For similar reasons, I would strongly
support just patching `__libcpp_allocate` directly, but I don't care to die on
that hill, so I made this patch as a ``-specific workaround.)
Repository:
rCXX libc++
https://reviews.llvm.org/D47344
Files:
src/experimental/memory_resource.cpp
Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "experimental/memory_resource"
+#include "memory"
#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
#include "atomic"
@@ -23,18 +24,36 @@
// new_delete_resource()
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;
+size_t space = 1;
+void *result = _VSTD::align(align, 1, p2, space);
+return (result == ptr);
+}
+#endif
+
class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
: public memory_resource
{
public:
~__new_delete_memory_resource_imp() = default;
protected:
virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+{
+void *result = _VSTD::__libcpp_allocate(__size, __align);
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__size != 0 && !is_aligned_to(result, __align)) {
+__throw_bad_alloc();
+}
+#endif
+return result;
+}
virtual void do_deallocate(void * __p, size_t, size_t __align)
-{ _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+{ _VSTD::__libcpp_deallocate(__p, __align); }
virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
{ return &__other == this; }
Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "experimental/memory_resource"
+#include "memory"
#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
#include "atomic"
@@ -23,18 +24,36 @@
// new_delete_resource()
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;
+size_t space = 1;
+void *result = _VSTD::align(align, 1, p2, space);
+return (result == ptr);
+}
+#endif
+
class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
: public memory_resource
{
public:
~__new_delete_memory_resource_imp() = default;
protected:
virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+{
+void *result = _VSTD::__libcpp_allocate(__size, __align);
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__size != 0 && !is_aligned_to(result, __align)) {
+__throw_bad_alloc();
+}
+#endif
+return result;
+}
virtual void do_deallocate(void * __p, size_t, size_t __align)
-{ _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+{ _VSTD::__libcpp_deallocate(__p, __align); }
virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
{ return &__other == this; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "experimental/memory_resource"
+#include "memory"
#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
#include "atomic"
@@ -23,18 +24,37 @@
// new_delete_resource()
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;
+size_t space = 1;
+void *result = _VSTD::align(align, 1, p2, space);
+return (result == ptr);
+}
+#endif
+
class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
: public memory_resource
{
public:
~__new_delete_memory_resource_imp() = default;
protected:
virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+{
+void *result = _VSTD::__libcpp_allocate(__size, __align);
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__size != 0 && !is_aligned_to(result, __align)) {
+_VSTD::__libcpp_deallocate(result, __align);
+__throw_bad_alloc();
+}
+#endif
+return result;
+}
virtual void do_deallocate(void * __p, size_t, size_t __align)
-{ _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+{ _VSTD::__libcpp_deallocate(__p, __align); }
virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
{ return &__other == this; }
Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "experimental/memory_resource"
+#include "memory"
#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
#include "atomic"
@@ -23,18 +24,37 @@
// new_delete_resource()
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;
+size_t space = 1;
+void *result = _VSTD::align(align, 1, p2, space);
+return (result == ptr);
+}
+#endif
+
class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
: public memory_resource
{
public:
~__new_delete_memory_resource_imp() = default;
protected:
virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+{
+void *result = _VSTD::__libcpp_allocate(__size, __align);
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__size != 0 && !is_aligned_to(result, __align)) {
+_VSTD::__libcpp_deallocate(result, __align);
+__throw_bad_alloc();
+}
+#endif
+return result;
+}
virtual void do_deallocate(void * __p, size_t, size_t __align)
-{ _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+{ _VSTD::__libcpp_deallocate(__p, __align); }
virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
{ return &__other == this; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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:
rCXX libc++
https://reviews.llvm.org/D47111
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp
@@ -0,0 +1,408 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class monotonic_buffer_resource
+
+#include
+#include
+#include
+#include
+
+#include "count_new.hpp"
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+struct repointable_resource : public std::experimental::pmr::memory_resource
+{
+std::experimental::pmr::memory_resource *which;
+
+explicit repointable_resource(std::experimental::pmr::memory_resource *res) : which(res) {}
+
+protected:
+virtual void *do_allocate(size_t size, size_t align)
+{ return which->allocate(size, align); }
+
+virtual void do_deallocate(void *p, size_t size, size_t align)
+{ return which->deallocate(p, size, align); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &rhs) const noexcept
+{ return which->is_equal(rhs); }
+};
+
+void test_construction_with_default_resource()
+{
+std::experimental::pmr::memory_resource *expected = std::experimental::pmr::null_memory_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+char buffer[16];
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2(16);
+std::experimental::pmr::monotonic_buffer_resource r3(buffer, sizeof buffer);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+assert(r3.upstream_resource() == expected);
+}
+
+expected = std::experimental::pmr::new_delete_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+char buffer[16];
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2(16);
+std::experimental::pmr::monotonic_buffer_resource r3(buffer, sizeof buffer);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+assert(r3.upstream_resource() == expected);
+}
+}
+
+void test_construction_without_buffer()
+{
+// Constructing a monotonic_buffer_resource should not cause allocations
+// by itself; the resource should wait to allocate until an allocation is
+// requested.
+
+globalMemCounter.reset();
+std::experimental::pmr::set_default_resource(std::experimental::pmr::new_delete_resource());
+
+std::experimental::pmr::monotonic_buffer_resource r1;
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::monotonic_buffer_resource r2(1024);
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::monotonic_buffer_resource r3(1024, std::experimental::pmr::new_delete_resource());
+assert(globalMemCounter.checkNewCalledEq(0));
+}
+
+void test_equality()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::monotonic_buffer_resource mono1;
+std::experimental::pmr::memory_resource & r1 = mono1;
+assert_on_compare c;
+std::experimental::pmr::memory_reso
[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 libc++
https://reviews.llvm.org/D47358
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.pool/synchronized_pool.pass.cpp
test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
test/support/count_new.hpp
Index: test/support/count_new.hpp
===
--- test/support/count_new.hpp
+++ test/support/count_new.hpp
@@ -211,6 +211,11 @@
return disable_checking || n != delete_called;
}
+bool checkDeleteCalledGreaterThan(int n) const
+{
+return disable_checking || delete_called > n;
+}
+
bool checkAlignedNewCalledEq(int n) const
{
return disable_checking || n == aligned_new_called;
Index: test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
@@ -0,0 +1,203 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class unsynchronized_pool_resource
+
+#include
+#include
+#include
+#include
+
+#include "count_new.hpp"
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+static bool is_aligned_to(void *p, size_t alignment)
+{
+void *p2 = p;
+size_t space = 1;
+void *result = std::align(alignment, 1, p2, space);
+return (result == p);
+}
+
+void test_construction_with_default_resource()
+{
+std::experimental::pmr::memory_resource *expected = std::experimental::pmr::null_memory_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+std::experimental::pmr::pool_options opts { 0, 0 };
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2(opts);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+}
+
+expected = std::experimental::pmr::new_delete_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+std::experimental::pmr::pool_options opts { 1024, 2048 };
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2(opts);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+}
+}
+
+void test_construction_does_not_allocate()
+{
+// Constructing a unsynchronized_pool_resource should not cause allocations
+// by itself; the resource should wait to allocate until an allocation is
+// requested.
+
+globalMemCounter.reset();
+std::experimental::pmr::set_default_resource(std::experimental::pmr::new_delete_resource());
+
+std::experimental::pmr::unsynchronized_pool_resource r1;
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::unsynchronized_pool_resource r2(std::experimental::pmr::pool_options{ 1024, 2048 });
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::unsynchronized_pool_resource r3(std::experimental::pmr::pool_options{ 1024, 2048 }, std::experimental::pmr::new_delete_resource());
+assert(globalMemCounter.checkNewCalledEq(0));
+}
+
+void test_equality()
+{
+// Same object
+{
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::unsynchronized_pool_resource unsync1;
+std::experimental::pmr::memory_resource & r1 = unsync1;
+
[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 https://reviews.llvm.org/D47344 https://reviews.llvm.org/D47111 https://reviews.llvm.org/D47358 https://reviews.llvm.org/D47360. Repository: rCXX libc++ https://reviews.llvm.org/D47090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 due to this change, hooray). Repository: rC Clang https://reviews.llvm.org/D47067 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 for the sake of argument that that's right, it wouldn't buy us
anything here because of padding, would it?
At the moment, `__alignment_` needs to have enough range to store the `align`
parameter passed to `monotonic_buffer_resource::do_allocate`. In an SSE world
we should not blithely assume that `align < 256`. We //could// store
`lg(align)` in a single byte since `align` is always a power of two and less
than 2^64 — but then we're back to the first argument, which is that it'll be
padded to 8 bytes anyway so what do we gain.
Comment at: include/experimental/memory_resource:474
+protected:
+void* do_allocate(size_t __bytes, size_t __alignment);
+
EricWF wrote:
> Lets add `override` to these functions.
I grepped for "override" in `include/` and saw exactly one (accidental?) use in
`experimental/filesystem`, so I thought probably libc++ had a policy not to use
it for portability reasons. I'll add it throughout, but would like someone to
explicitly confirm that
(A) it's okay to use in this header and wouldn't need to be guarded with a
`_LIBCPP_OVERRIDE` macro or anything
(B) Arthur should //not// bother trying to add it to any //other// headers, not
even "for consistency"
Comment at: src/experimental/memory_resource.cpp:217
+{
+if (void *result = try_allocate_from_chunk(&__original_, bytes, align)) {
+return result;
EricWF wrote:
> Drop the braces for conditionals and loops with single statements.
*shiver* but okay.
Comment at: src/experimental/memory_resource.cpp:237
+
+void *result = __res_->allocate(aligned_capacity, align);
+__monotonic_buffer_header *header = (__monotonic_buffer_header *)((char
*)result + aligned_capacity - header_size);
For reference, here is where we ask the upstream for a block aligned according
to the user's `align`.
It occurs to me that the upstream might not be able to satisfy such a request
(actually, `new_delete_resource` works that way for me because libc++ doesn't
support aligned new and delete on OSX), which would make the upstream throw
`bad_alloc`. We handle this by passing along the exception. We //could//
conceivably handle it by retrying with
```
aligned_capacity += align;
__res_->allocate(aligned_capacity, alignof(max_align_t));
header->__alignment_ = alignof(max_align_t);
```
but I'm not sure that that's any of `monotonic_buffer_resource`'s business.
Happy to make the patch if you think it ought to be.
Repository:
rCXX libc++
https://reviews.llvm.org/D47111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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, then they are //probably// intending that
our first upstream-allocation be big enough to serve at least `initial_size`
1-byte allocations (or one `initial_size`-byte allocation with a suitably small
alignment). This is explicitly //not// mandated by the Standard (it merely
requires that our next upstream-allocation be of size at least `initial_size`),
but it's probably a healthy choice.
Repository:
rCXX libc++
https://reviews.llvm.org/D47111
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/copy_move.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/with_default_resource.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/without_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_initial_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_zero_sized_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
@@ -0,0 +1,13 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
@@ -0,0 +1,61 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class monotonic_buffer_resource
+
+#include
+#include
+#include
+#include
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+int main()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::monotonic_buffer_resource mono1;
+std::experimental::pmr::memory_resource & r1 = mono1;
+assert_on_compare c;
+std::experimental::pmr::memory_resource & r2 = c;
+assert(r1 != r2);
+assert(!(r1 == r2));
+}
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffe
[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, `sizeof(monotonic_buffer_resource) == 48` and the overhead
per allocated chunk is `24` bytes.
Eric suggests replacing `size_t __align_` with `uint8_t __log2_align_`. I'm
amenable to this idea, but I'd want to know what's the best way to compute the
log2 of a user-supplied number.
Repository:
rCXX libc++
https://reviews.llvm.org/D47111
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/copy_move.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/with_default_resource.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/without_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_initial_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_zero_sized_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
@@ -0,0 +1,13 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
@@ -0,0 +1,61 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class monotonic_buffer_resource
+
+#include
+#include
+#include
+#include
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+int main()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::monotonic_buffer_resource mono1;
+std::experimental::pmr::memory_resource & r1 = mono1;
+assert_on_compare c;
+std::experimental::pmr::memory_resource & r2 = c;
+assert(r1 != r2);
+assert(!(r1 == r2));
+}
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
===
--- /dev/null
+++ te
[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
test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp
test/std/experimental/memory/memory.resource.pool/synchronized_pool.pass.cpp
test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
test/support/count_new.hpp
Index: test/support/count_new.hpp
===
--- test/support/count_new.hpp
+++ test/support/count_new.hpp
@@ -211,6 +211,11 @@
return disable_checking || n != delete_called;
}
+bool checkDeleteCalledGreaterThan(int n) const
+{
+return disable_checking || delete_called > n;
+}
+
bool checkAlignedNewCalledEq(int n) const
{
return disable_checking || n == aligned_new_called;
Index: test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.pool/unsynchronized_pool.pass.cpp
@@ -0,0 +1,203 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class unsynchronized_pool_resource
+
+#include
+#include
+#include
+#include
+
+#include "count_new.hpp"
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+static bool is_aligned_to(void *p, size_t alignment)
+{
+void *p2 = p;
+size_t space = 1;
+void *result = std::align(alignment, 1, p2, space);
+return (result == p);
+}
+
+void test_construction_with_default_resource()
+{
+std::experimental::pmr::memory_resource *expected = std::experimental::pmr::null_memory_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+std::experimental::pmr::pool_options opts { 0, 0 };
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2(opts);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+}
+
+expected = std::experimental::pmr::new_delete_resource();
+std::experimental::pmr::set_default_resource(expected);
+{
+std::experimental::pmr::pool_options opts { 1024, 2048 };
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2(opts);
+assert(r1.upstream_resource() == expected);
+assert(r2.upstream_resource() == expected);
+}
+}
+
+void test_construction_does_not_allocate()
+{
+// Constructing a unsynchronized_pool_resource should not cause allocations
+// by itself; the resource should wait to allocate until an allocation is
+// requested.
+
+globalMemCounter.reset();
+std::experimental::pmr::set_default_resource(std::experimental::pmr::new_delete_resource());
+
+std::experimental::pmr::unsynchronized_pool_resource r1;
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::unsynchronized_pool_resource r2(std::experimental::pmr::pool_options{ 1024, 2048 });
+assert(globalMemCounter.checkNewCalledEq(0));
+
+std::experimental::pmr::unsynchronized_pool_resource r3(std::experimental::pmr::pool_options{ 1024, 2048 }, std::experimental::pmr::new_delete_resource());
+assert(globalMemCounter.checkNewCalledEq(0));
+}
+
+void test_equality()
+{
+// Same object
+{
+std::experimental::pmr::unsynchronized_pool_resource r1;
+std::experimental::pmr::unsynchronized_pool_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::unsynchronized_pool_resource unsync1;
+std::experimental::pmr::memory_resource & r1 = unsync1;
+assert_on_compare c;
+std::experimental::pmr::memory_resource & r2 = c;
+a
[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 of a `const pool_options` object actually depends on a DR, so I needed to add `UNSUPPORTED: apple-clang-7`. I don't know how to determine the list of all unsupporty targets. Repository: rCXX libc++ https://reviews.llvm.org/D47358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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()." Are you 100% sure that you're not just a person with broken code? In other words, what did this guy from 2013 get wrong? -- or, if "he got nothing wrong", then why can't you just follow his advice to eliminate the duplicate typeinfos from your code? http://www.russellmcc.com/posts/2013-08-03-rtti.html Repository: rL LLVM https://reviews.llvm.org/D38599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51200: Introduce per-callsite inline intrinsics
Quuxplusone added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8236
+def err_argument_to_inline_intrinsic_builtin_call : Error<
+ "argument to %0 must not be a builtin call">;
+def err_argument_to_inline_intrinsic_pdotr_call : Error<
I'd expect to be able to write
__builtin_always_inline(sqrt(x))
__builtin_no_inline(sqrt(x))
without caring whether `sqrt` was a real function or just a macro around
`__builtin_sqrt`. How important is it that calls to builtin functions be
errors, instead of just being ignored for this purpose?
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8238
+def err_argument_to_inline_intrinsic_pdotr_call : Error<
+ "argument to %0 must not be a pseudo-destructor call">;
+def err_argument_to_inline_intrinsic_block_call : Error<
Spelling: `s/dotr/dtor/`
Semantics: Does this trigger in generic contexts? I feel like I'd want to be
able to write
__builtin_always_inline(p->~T());
without caring whether `T` was a primitive type or not. How important is it
that pseudo-destructor calls be errors, instead of just being ignored for this
purpose?
Comment at: include/clang/CodeGen/CGFunctionInfo.h:713
+ CanQualType resultType, ArrayRef argTypes,
+ CallInlineKind inlineKind = CallInlineKind::DefaultInline) {
ID.AddInteger(info.getCC());
Here and throughout, wouldn't it be more traditional to name the parameter
variable `CIK`? (You have sometimes `CIK`, sometimes `inlineKind`, sometimes
`InlineKind`.)
It also feels needlessly convoluted to have a value of type CallInlineKind
stored in a member named InlineCall (note the reversal of the words), but I'm
not sure how that's generally dealt with.
Comment at: test/SemaCXX/inline-builtins.cpp:12
+ void operator++();
+ friend S operator+(const S &, const S &);
+};
This raises a question for me about the semantics of "always inlining" a "call":
struct A { A(); A(A&&); };
struct B { B(A); }
void foo(B);
__builtin_always_inline(foo(A{}));
Does `__builtin_always_inline` apply to `foo`, `B(A)` (used to create the
parameter to `foo`), `A()` (used to create the argument to `A(A&&)`), or all of
the above?
You should add a test case something like this, maybe with multiple function
arguments.
Comment at: test/SemaCXX/inline-builtins.cpp:71
+ S s2 = __builtin_always_inline(1_s); // expected-error {{argument to
__builtin_always_inline must be a function, member function, or operator call}}
+ s2 = __builtin_no_inline(1_s); // expected-error {{argument to
__builtin_no_inline must be a function, member function, or operator call}}
+
Really good you thought of this case! But shouldn't this *not* be an error?
`1_s` is an operator call.
Repository:
rC Clang
https://reviews.llvm.org/D51200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"
Quuxplusone added a comment. @EricWF ping? Repository: rCXX libc++ https://reviews.llvm.org/D47344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"
Quuxplusone updated this revision to Diff 156684.
Quuxplusone added a comment.
Fix the "zero-byte allocation might be misaligned" issue by just never trying
to allocate zero bytes — try to allocate 1 byte instead.
Repository:
rCXX libc++
https://reviews.llvm.org/D47344
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "experimental/memory_resource"
+#include "memory"
#ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
#include "atomic"
@@ -23,38 +24,51 @@
// new_delete_resource()
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;
+size_t space = 1;
+void *result = _VSTD::align(align, 1, p2, space);
+return (result == ptr);
+}
+#endif
+
class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
: public memory_resource
{
-public:
-~__new_delete_memory_resource_imp() = default;
-
-protected:
-virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+void *do_allocate(size_t bytes, size_t align) override
+{
+#ifndef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+return _VSTD::__libcpp_allocate(bytes, align);
+#else
+if (bytes == 0)
+bytes = 1;
+void *result = _VSTD::__libcpp_allocate(bytes, align);
+if (!is_aligned_to(result, align)) {
+_VSTD::__libcpp_deallocate(result, align);
+__throw_bad_alloc();
+}
+return result;
+#endif
+}
-virtual void do_deallocate(void * __p, size_t, size_t __align)
-{ _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+void do_deallocate(void *p, size_t, size_t align) override
+{ _VSTD::__libcpp_deallocate(p, align); }
-virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-{ return &__other == this; }
+bool do_is_equal(const memory_resource& other) const _NOEXCEPT override
+{ return &other == this; }
};
// null_memory_resource()
class _LIBCPP_TYPE_VIS __null_memory_resource_imp
: public memory_resource
{
-public:
-~__null_memory_resource_imp() = default;
-
-protected:
-virtual void* do_allocate(size_t, size_t) {
-__throw_bad_alloc();
-}
-virtual void do_deallocate(void *, size_t, size_t) {}
-virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-{ return &__other == this; }
+void *do_allocate(size_t, size_t) override { __throw_bad_alloc(); }
+void do_deallocate(void *, size_t, size_t) override {}
+bool do_is_equal(const memory_resource& other) const _NOEXCEPT override
+{ return &other == this; }
};
namespace {
Index: include/experimental/memory_resource
===
--- include/experimental/memory_resource
+++ include/experimental/memory_resource
@@ -195,7 +195,7 @@
}
_LIBCPP_INLINE_VISIBILITY
-void deallocate(_ValueType * __p, size_t __n) _NOEXCEPT {
+void deallocate(_ValueType * __p, size_t __n) {
_LIBCPP_ASSERT(__n <= __max_size(),
"deallocate called for size which exceeds max_size()");
__res_->deallocate(__p, __n * sizeof(_ValueType), alignof(_ValueType));
@@ -265,7 +265,7 @@
template
_LIBCPP_INLINE_VISIBILITY
-void destroy(_Tp * __p) _NOEXCEPT
+void destroy(_Tp * __p)
{ __p->~_Tp(); }
_LIBCPP_INLINE_VISIBILITY
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"
Quuxplusone marked 5 inline comments as done.
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 wrote:
> Quuxplusone wrote:
> > EricWF wrote:
> > > Also, I'm not sure about the `size != 0` check. The returned pointer may
> > > be non-null and incorrectly aligned.
> > This was covered in my commit message/summary: "If n == 0, return an
> > unspecified result."
> > However, I am chagrined to state that I have no idea where I got that idea!
> > I can't find support for that anywhere in the current Standard (although
> > I'm not sure if I missed it).
> >
> > We must choose here between "allocating zero bytes sometimes returns an
> > underaligned pointer" and "allocating zero bytes sometimes throws
> > bad_alloc." Neither one seems like good QoI. But as I no longer see why I
> > thought "underaligned pointer" was permitted, I will change it to
> > "sometimes throws bad_alloc" and re-upload.
> Hm! This and your other comment play into each other unfortunately well. When
> `size == 0`, the pointer we get back from `__libcpp_allocate` actually points
> to zero bytes, which means that when I call `std::align` on it, I invoke
> undefined behavior.
>
> My new theory is "I don't care about that undefined behavior." I'd still
> rather take undefined-behavior-in-a-rare-case over
> implementation-defined-behavior-in-all-cases, and I'm too lazy to implement
> implementation-defined-behavior-only-in-a-rare-case unless you tell me to. :P
Now fixed. (The fix is always in the last place you look!)
Repository:
rCXX libc++
https://reviews.llvm.org/D47344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49771: CodeGen: use non-zero memset when possible for automatic variables
Quuxplusone added inline comments.
Comment at: test/CodeGen/init.c:202
+ union U { char c; int i; };
+ struct S { union U u; int i; };
+ struct S arr[5] = { { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, {
{0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, { {0xF0}, 0xF0F0F0F0 }, };
Drive-by suggestion: If you make this `struct S { union U u; short s; };` then
you'll also be testing the case of "padding between struct fields", which is
otherwise untested here.
Repository:
rC Clang
https://reviews.llvm.org/D49771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.
Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.
Comment at: include/vector:296
+_LIBCPP_INLINE_VISIBILITY
+inline void
+__copy_construct_forward(_Alloc& __a, _Iter __begin1, _Iter __end1,
ldionne wrote:
> Do you really need `inline` here?
I'm actually not sure — and also suddenly not sure if the visibility attribute
should be `_LIBCPP_INLINE_VISIBILITY` or `_LIBCPP_TEMPLATE_VIS`. (I *think* the
latter is only for type templates, though, not function templates?) However,
this is exactly parallel to what we do for `operator<`, so I think changing it
would be gratuitous. If someone wants to remove `inline` from a bunch of
templates, I won't complain, but I also don't want this PR to be the one that
initiates it.
```
template
inline _LIBCPP_INLINE_VISIBILITY
bool
operator< (const vector<_Tp, _Allocator>& __x, const vector<_Tp, _Allocator>&
__y)
{
return _VSTD::lexicographical_compare(__x.begin(), __x.end(), __y.begin(),
__y.end());
}
```
Comment at: include/vector:545
+is_trivially_move_constructible<_Tp>::value
+> {};
+
Louis writes:
> It would be nice if all the TMP required to determine whether to call
> `__move_construct_forward(..., true_type)` or `__move_construct_forward(...,
> false_type)` was done in `__move_construct_forward` itself (or a helper).
> This way, callers wouldn't have to do it themselves.
I know where you're coming from, but I believe that in this case we definitely
*can't* do that, because the whole point of these routines is that the routine
itself can't always tell whether it's supposed to memcpy or not; the *caller*
is the only one with the power to decide that. The decision (in theory, though
not yet in practice, because this particular PR is a pure refactoring) depends
not only on details of `_Tp` and `_Allocator` but also on the specific
call-site: we can memcpy more aggressively at some call-sites than others,
because of information available only to the caller (such as "this is a
relocation operation").
See
https://github.com/Quuxplusone/libcxx/commit/e7e5999b01#diff-07c2b769648850d040dcbb07754e5f2fR1076
, lines 1076 et seq., for how I envision some future caller making the
decisions on a callsite-by-callsite basis.
Repository:
rCXX libc++
https://reviews.llvm.org/D49317
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.
Quuxplusone added a comment. @ldionne: I don't know if your "LGTM" is necessarily sufficient to commit this or not; but either way, I don't have commit privs, so could I ask you (or someone else) to commit this on my behalf? Thanks! Repository: rCXX libc++ https://reviews.llvm.org/D49317 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 it impossible to "constexp-ify" this code. Well, I have three thoughts on that. (A), "removing the calls to memcpy" sounds like you want to just call the *actual* move-constructor in a loop, and then later call the actual destructor in a loop. Which is to say, you don't want libc++ to have a codepath for this speed optimization at all. That's just leaving a ton of performance on the table, and I strongly disagree with that idea. (B), regardless, couldn't you achieve that goal simply by taking this patch almost exactly as it is except removing the overloads that take `true_type`? If you want constexpr-friendliness badly enough that you're willing to call the move-constructor and destructor even of trivially copyable types, then you can still use this framework; you just have to remove the overloads that call memcpy. That wouldn't be a major refactoring. (C), surely if you want the best of both worlds, you should be pushing someone to invent a constexpr memcpy and/or a way to [detect constexpr-context at compile time](https://quuxplusone.github.io/blog/2018/06/12/perennial-impossibilities/#detect-the-constexprness-of-the-current-context)? I don't think it makes sense to pessimize existing (non-constexpr) users in C++03-through-C++17 just because someone hypothetically might in C++2a-or-later want to mutate a std::vector in a constexpr context. Repository: rCXX libc++ https://reviews.llvm.org/D49317 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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. Having calls to memcpy in the code > > > paths makes it impossible to "constexp-ify" this code. > > > > Well, I have three thoughts on that. > > > > (A), "removing the calls to memcpy" sounds like you want to just call the > > *actual* move-constructor in a loop, and then later call the actual > > destructor in a loop. Which is to say, you don't want libc++ to have a > > codepath for this speed optimization at all. That's just leaving a ton of > > performance on the table, and I strongly disagree with that idea. > > > > (B), regardless, couldn't you achieve that goal simply by taking this patch > > almost exactly as it is except removing the overloads that take > > `true_type`? If you want constexpr-friendliness badly enough that you're > > willing to call the move-constructor and destructor even of trivially > > copyable types, then you can still use this framework; you just have to > > remove the overloads that call memcpy. That wouldn't be a major refactoring. > > > > (C), surely if you want the best of both worlds, you should be pushing > > someone to invent a constexpr memcpy and/or a way to [detect > > constexpr-context at compile > > time](https://quuxplusone.github.io/blog/2018/06/12/perennial-impossibilities/#detect-the-constexprness-of-the-current-context)? > > I don't think it makes sense to pessimize existing (non-constexpr) users > > in C++03-through-C++17 just because someone hypothetically might in > > C++2a-or-later want to mutate a std::vector in a constexpr context. > > Which is to say, you don't want libc++ to have a codepath for this speed > > optimization at all. > > You're completely correct. I don't want libc++ to have such a code path. > I want clang to generate a `memcpy` from the code w/o ever mentioning > `memcpy` in the source. > > @mclow.lists: So would you accept a version of this patch that simply removed the `true_type` overloads? That would change this from a pure refactoring to a performance regression, but it would still reduce the overall diff between libc++ master and libc++ trivially-relocatable. (Maybe it's no longer clear and needs restating: This patch is //currently// a pure refactoring. All I'm doing is moving the //existing// helper functions out of allocator_traits. IIUC, your objections apply to the existence of these //existing// helper functions just as much as to the refactored versions.) Repository: rCXX libc++ https://reviews.llvm.org/D49317 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 `__move_construct_forward` & friends functions from > `allocator_traits` to private static member functions of `std::vector` > (because they're only used in `std::vector` right now). > 2. keep the SFINAE on the allocator and avoid encoding any `memcpy` decision > at the call site. FWLIW, I approve of (1) but not (2), for the previously stated reason that the optimal path is known only at the call-site; the callee doesn't have enough information to know whether memcpy is appropriate. (But it sounds like Marshall doesn't want any memcpy happening at all, so maybe it's moot?) > However, an even better alternative would be to look into adding an overload > to `uninitialized_move` & friends that takes an allocator. We could then be > clever in how this is implemented. The major benefit I see here is that there > would be one common code path to optimize, as opposed to a > `std::vector`-specific code path. Yes, when I implemented https://github.com/Quuxplusone/from-scratch/, one of the many things I noticed was that none of the uninitialized_foo algorithms were useful out of the box; every one of them needed to be reimplemented to take an allocator parameter. (A.k.a., "scoped_allocator_adaptor is why we can't have nice things.") However, as you point out, this is a long-standing problem and would require a library paper to do "right." (It would still be easy enough to add the needed algorithms with uglified names, e.g. `__uninitialized_copy_a`, `__destroy_a`, etc. This is exactly what libstdc++ does, and libc++ might be wise to copy its approach.) I'd be happy to throw together a patch for `__uninitialized_copy_a` etc., since I think that would improve libc++ in general; but I don't see how that would directly help any specific short-term problem in libc++. This patch as it is helps two specific short-term problems: (1) that user specializations of allocator_traits don't work (but, as the test case comments, this is arguably not a good idea anyway; see also https://quuxplusone.github.io/blog/2018/07/14/traits-classes/ ) (2) that the diff between libc++ trunk and libc++ trivially-relocatable is unnecessarily large Messing with the uninitialized_foo algorithms would not directly help either of these problems, so we'd have to come up with some other rationale for it. Repository: rCXX libc++ https://reviews.llvm.org/D49317 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41458: [libc++][C++17] Elementary string conversions for integral types
Quuxplusone added a comment. @lichray: I'm interested in merging this patch into my libc++ fork, but the latest update seems to be missing a ton of files. For example `charconv_test_helpers.h` is included by one of the tests, but does not exist. Also there's a lot of boilerplate missing: you ought to add the new header to module.modulemap, and to double_include.sh.cpp, and so on. Might you upload a new patch anytime soon? Repository: rCXX libc++ https://reviews.llvm.org/D41458 ___ 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 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
https://quuxplusone.github.io/blog/2018/07/18/announcing-trivially-relocatable/
.
There are two parts to this compiler support:
- the type trait `__is_trivially_relocatable(T)`, which is similar in spirit to
`__is_trivially_destructible(T)`, in that it lets the programmer access
information that the compiler itself already knows; and
- the warranting attribute `[[trivially_relocatable]]`, which is similar in
spirit to `[[trivial_abi]]`, in that it lets the programmer communicate back to
the compiler that a certain user-defined type should be //assumed// to have
this property even though it would not //naturally// have the property all else
being equal.
The official home of this branch thus far has been
https://github.com/Quuxplusone/clang/tree/trivially-relocatable , but I figure
that a Clang review would be a good way to get some more eyeballs on it, plus,
if we can get it into Clang trunk then I wouldn't have to keep rebasing it
every week.
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
include/clang/Sema/Sema.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,478 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is either
+// not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete
[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"
Quuxplusone updated this revision to Diff 158613.
Quuxplusone added a comment.
Address feedback from @Rakete. Fix wrong ordering of 'class|struct|...' in
the diagnostic. Add `union` test cases. Correctly drop the attribute whenever
it is diagnosed as inapplicable.
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
compiler-explorer-llvm-commit.sh
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
include/clang/Sema/Sema.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,495 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is either
+// not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) = default; };
+// expected-error@-1{{cannot be applied to
[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"
Quuxplusone added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2942
+ "not %select{move-constructible|destructible}2"
+ >;
+
> Might be useful to add a note explaining why the type isn't trivially
> relocatable instead of the general "because it is not destructible".
You mean, like, a series of notes pointing at "because its base class B is not
destructible... because B's destructor is defined as deleted here"? I agree
that might be helpful, but since it only happens when the programmer is messing
around with the attribute anyway, I wouldn't want to do anything too innovative
or LoC-consuming. I'd cut and paste ~10 lines of code from somewhere else that
already does something like that if you point me at it, but otherwise I think
it's not worth the number of extra codepaths that would need to be tested.
Comment at: include/clang/Sema/Sema.h:4304
+ bool IsTriviallyRelocatableType(QualType QT) const;
+
Rakete wrote:
> Any reason why this is a free function? Should be a member function of
> `QualType`.
`class QualType` is defined in `AST/`, whereas all the C++-specific
TriviallyRelocatable stuff is currently confined to `Sema/`. My impression was
that I should try to preserve that separation, even if it meant being ugly
right here. I agree that this is a stupid hack, and would love to get rid of
it, but I think I need some guidance as to how much mixing of `AST` and `Sema`
is permitted.
Comment at: lib/Sema/SemaDeclCXX.cpp:6066
+if (M->hasAttr() || Record->hasAttr()) {
+ // Consider removing this case to simplify the Standard wording.
+} else {
Rakete wrote:
> This should be a `// TODO: ...`. Is this comment really appropriate? The
> intended audience isn't compiler writers I think.
Similar to the `//puts` comments, this comment is appropriate for me but not
for Clang. :) Will replace with a comment containing the actual proposed
wording.
Comment at: lib/Sema/SemaDeclCXX.cpp:6157
+
+ if (getLangOpts().CPlusPlus11 &&
+ !Record->hasAttr() &&
Rakete wrote:
> This really just checks whether the type has defaulted copy constructor. If
> there was a move constructor, it would have been handled above. If the
> copy/move constructor is implicitly deleted, it would have been handled also
> above. Please simplify this accordingly.
Can you elaborate on this one? I assume you mean that some of lines 6157
through 6171 are superfluous, but I can't figure out which ones or how to
simplify it.
Comment at: lib/Sema/SemaDeclCXX.cpp:6158
+ if (getLangOpts().CPlusPlus11 &&
+ !Record->hasAttr() &&
+ Record->isTriviallyRelocatable()) {
Rakete wrote:
> Why do you need to check whether the attribute is present or not? This is
> supposed to be whether the type is naturally trivially relocatable, so the
> presence of the attribute is not important.
I just thought that checking the presence of the attribute would be cheaper
than doing these lookups, so I was trying to short-circuit it. However, you are
correct for two reasons:
- The check passes 99.99% of the time anyway, so it's *everyone* paying a small
cost just so that the 0.01% can be a bit faster. This is a bad tradeoff.
- Skipping the check when the attribute is present is actually incorrect, in
the case that the type is not relocatable and the attribute is going to be
dropped. (It was not dropped in this revision. It will be dropped in the next
revision.) In that case, even with the attribute, we still want to execute this
codepath.
Comment at: lib/Sema/SemaDeclCXX.cpp:6656
SetDeclDeleted(MD, MD->getLocation());
+ if (CSM == CXXMoveConstructor || CSM == CXXDestructor) {
+//puts("because 6646");
Rakete wrote:
> You don't actually need those three lines. This is already handled in
> `CheckCompletedCXXClass`.
Confirmed. Nice!
Comment at: test/SemaCXX/trivially-relocatable.cpp:42
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first
declaration}}
Rakete wrote:
> Why does this restriction exist? None of the existing attributes have it and
> I don't see why it would make sense to disallow this.
`[[noreturn]]` has it, and I am pretty sure that `[[trivial_abi]]` *ought* to
have it, even though it currently doesn't. The intent is to make it harder for
people to create ODR violations by declaring a type, using it in some way, and
then saying "oh by the way this type was x all along."
```
struct S { S(S&&); ~S(); };
std::vector vec;
struct [[trivially_relocatable]] S; // ha ha, now you have to re-do all of
vector's codegen!
```
Does that make sense as a reason it would be (mildly) beneficial to have thi
[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"
Quuxplusone updated this revision to Diff 158615.
Quuxplusone added a comment.
clang-format.
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
compiler-explorer-llvm-commit.sh
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
include/clang/Sema/Sema.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,495 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is either
+// not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) = default; };
+// expected-error@-1{{cannot be applied to struct 'D6' because it is not move-constructible}}
+
+template
+struct [[trivially_relocatable]] DT1 : private T { }; // ok
+
+struct D7 {
+DT1 m;
+};
+
+class [[trivially_
[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
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/AST/Type.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,495 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is either
+// not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) = default; };
+// expected-error@-1{{cannot be applied to struct 'D6' because it is not move-constructible}}
+
+template
+struct [[trivially_relocatable]] DT1 : private T { }; // o
[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 pipework for the Compiler Explorer integration. I manually deleted
it from the previously uploaded diff, but forgot to delete it this time. You
can ignore it.
Comment at: include/clang/Sema/Sema.h:4304
+ bool IsTriviallyRelocatableType(QualType QT) const;
+
Rakete wrote:
> Quuxplusone wrote:
> > Rakete wrote:
> > > Any reason why this is a free function? Should be a member function of
> > > `QualType`.
> > `class QualType` is defined in `AST/`, whereas all the C++-specific
> > TriviallyRelocatable stuff is currently confined to `Sema/`. My impression
> > was that I should try to preserve that separation, even if it meant being
> > ugly right here. I agree that this is a stupid hack, and would love to get
> > rid of it, but I think I need some guidance as to how much mixing of `AST`
> > and `Sema` is permitted.
> Nah, it's fine. There are lots of C++ specific things in AST/, because the
> AST nodes represent C++-y stuff. Trivially copyable is also part of
> `QualType`, even though it's C++ specific.
Okay, moved!
Comment at: lib/Sema/SemaDeclAttr.cpp:6481
break;
+ case ParsedAttr::AT_TriviallyRelocatable:
+handleTriviallyRelocatableAttr(S, D, AL);
Rakete wrote:
> Why is this attribute under "Microsoft Attributes"? ;)
I dunno, the random `//comments` seem pretty silly to me. There's nothing
"Microsoft" about TrivialABI either (and no reason anyone should care, in this
context, that EmptyBases or LayoutVersion are "Microsoft"). I just put it here
because it's somewhat related to TrivialABI — and/or, to reduce code churn if
anyone ever decides to Do The Right Thing and alphabetize these switch cases.
Comment at: lib/Sema/SemaDeclCXX.cpp:6187
+Record->dropAttr();
+ } else if (Record->needsImplicitMoveConstructor() &&
+ Record->defaultedMoveConstructorIsDeleted()) {
Rakete wrote:
> This is dead code. `Record` never needs an implicit move constructor at this
> point, because either 1) it never did or 2) it was defined above by
> `LookupSpecialMember`.
Confirmed that this code is never hit; and removed. Just for my own
information: you're saying that the call to `LookupSpecialMember` on line 6179,
even though it's looking up the //destructor//, will actually cause all the
`needsImplicitFootor` flags to get resolved? And so also I guess I should never
have been looking at those flags directly; I should have handled this case by
calling `LookupSpecialMember` like I do on line 6196. Is that basically correct?
Repository:
rC Clang
https://reviews.llvm.org/D50119
___
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 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 avoid two iterations of the
> fields.
Done. Btw, I notice that `ActOnFields` spends a lot of time doing
`dyn_cast(Record)` over and over. If I had commit privs, I'd
refactor it to compute `CXXRecordDecl *CXXRecord =
dyn_cast_or_null(Record);` once at the very top of the function,
and then use `CXXRecord` throughout.
Comment at: lib/Sema/SemaDeclCXX.cpp:6174
+ Record->hasAttr() &&
+ !isTemplateInstantiation(Record->getTemplateSpecializationKind())) {
+if (Record->getDefinition() && !Record->isDependentContext() &&
Rakete wrote:
> The call to `isTemplateInstantiation` is wrong. Consider:
>
> ```
> template
> struct [[trivially_relocatable]] A {
> T t;
> };
>
> struct X {
> X() = default;
> X(X &&) = delete;
> };
>
> A d;
> static_assert(!__is_trivially_relocatable(decltype(d))); // oops, fires
> ```
>
> There is also no diagnostic saying that `A` cannot be marked
> `[[trivially_relocatable]]`.
The absence of any diagnostics is intentional. We're saying that `A` is
trivially relocatable except-of-course-when-it's-not-relocatable-at-all,
similar to how templates currently work with `constexpr`.
However, the fact that `__is_trivially_relocatable(A)` when
`!__is_constructible(A, A&&)` does seem like a bug. I should probably
move the `isTemplateInstantiation` check down to control just the diagnostic,
eh?
Comment at: lib/Sema/SemaDeclCXX.cpp:6176
+if (Record->getDefinition() && !Record->isDependentContext() &&
+!Record->isBeingDefined()) {
+ // Check that the destructor is non-deleted.
Rakete wrote:
> `Record` is never being defined at this point, even for templates. It also
> always has a definition AFAIK.
Yes, that matches my observations so far (but I'm running the tests again to
confirm). I'd originally copied this formula from somewhere else, I forget
where.
Repository:
rC Clang
https://reviews.llvm.org/D50119
___
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 updated this revision to Diff 159073.
Quuxplusone marked 8 inline comments as done.
Quuxplusone added a comment.
Correctly drop the attribute from non-relocatable instantiations of class
templates (even though they don't print the diagnostic).
Repository:
rC Clang
https://reviews.llvm.org/D50119
Files:
docs/LanguageExtensions.rst
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/Features.def
include/clang/Basic/TokenKinds.def
include/clang/Basic/TypeTraits.h
lib/AST/ASTImporter.cpp
lib/AST/DeclCXX.cpp
lib/AST/Type.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExprCXX.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Lexer/has_extension_cxx.cpp
test/Misc/pragma-attribute-supported-attributes-list.test
test/SemaCXX/trivially-relocatable.cpp
Index: test/SemaCXX/trivially-relocatable.cpp
===
--- /dev/null
+++ test/SemaCXX/trivially-relocatable.cpp
@@ -0,0 +1,510 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// expected-diagnostics
+
+static_assert(__has_extension(trivially_relocatable), "");
+
+// It shall appear at most once in each attribute-list
+// and no attribute-argument-clause shall be present.
+
+struct [[trivially_relocatable, trivially_relocatable]] B1 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot appear multiple times in an attribute specifier}}
+
+struct [[trivially_relocatable]] [[trivially_relocatable]] B2 {}; // should really be an error
+
+struct [[trivially_relocatable(42)]] B3 {};
+// expected-error@-1{{attribute 'trivially_relocatable' cannot have an argument list}}
+
+
+// The first declaration of a type shall specify the
+// trivially_relocatable attribute if any declaration of that
+// type specifies the trivially_relocatable attribute.
+
+struct [[trivially_relocatable]] A1 {}; // ok
+struct [[trivially_relocatable]] A1;
+
+struct [[trivially_relocatable]] A2; // ok
+struct [[trivially_relocatable]] A2 {};
+
+struct [[trivially_relocatable]] A3 {}; // ok
+struct A3;
+
+struct [[trivially_relocatable]] A4; // ok
+struct A4 {};
+
+struct A5 {};
+struct [[trivially_relocatable]] A5;
+// expected-error@-1{{type A5 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+// expected-warning@-3{{attribute declaration must precede definition}}
+// expected-note@-5{{previous definition is here}}
+
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first declaration}}
+// expected-note@-3{{declaration missing 'trivially_relocatable' attribute is here}}
+
+
+// If a type T is declared with the trivially_relocatable attribute, and T is
+// either not move-constructible or not destructible, the program is ill-formed.
+
+struct NonDestructible {
+NonDestructible(const NonDestructible&) = default;
+NonDestructible(NonDestructible&&) = default;
+~NonDestructible() = delete;
+};
+struct NonCopyConstructible {
+NonCopyConstructible(const NonCopyConstructible&) = delete;
+};
+struct NonMoveConstructible {
+NonMoveConstructible(const NonMoveConstructible&) = default;
+NonMoveConstructible(NonMoveConstructible&&) = delete;
+};
+static_assert(!__is_trivially_relocatable(NonDestructible), "");
+static_assert(!__is_trivially_relocatable(NonCopyConstructible), "");
+static_assert(!__is_constructible(NonCopyConstructible, NonCopyConstructible&&), "");
+static_assert(!__is_trivially_relocatable(NonMoveConstructible), "");
+static_assert(!__is_constructible(NonMoveConstructible, NonMoveConstructible&&), "");
+
+struct [[trivially_relocatable]] D1 { ~D1() = delete; };
+// expected-error@-1{{cannot be applied to struct 'D1' because it is not destructible}}
+
+struct [[trivially_relocatable]] D2 : private NonDestructible { };
+// expected-error@-1{{cannot be applied to struct 'D2' because it is not destructible}}
+
+struct [[trivially_relocatable]] D3 { D3(const D3&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D3' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D4 { D4(const D4&) = default; D4(D4&&) = delete; };
+// expected-error@-1{{cannot be applied to struct 'D4' because it is not move-constructible}}
+
+struct [[trivially_relocatable]] D5 : private NonCopyConstructible { };
+// expected-error@-1{{cannot be applied to struct 'D5' because it is not move-constructible}}
+static_assert(!__is_constructible(D5, D5&&), "");
+
+struct [[trivially_relocatable]] D6 : private NonMoveConstructible { D6(D6&&) = default; };
+// expected-error@-1{{cannot be applied to struct 'D6' because it is not
[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 wrote:
> > Quuxplusone wrote:
> > > Rakete wrote:
> > > > This is dead code. `Record` never needs an implicit move constructor at
> > > > this point, because either 1) it never did or 2) it was defined above
> > > > by `LookupSpecialMember`.
> > > Confirmed that this code is never hit; and removed. Just for my own
> > > information: you're saying that the call to `LookupSpecialMember` on line
> > > 6179, even though it's looking up the //destructor//, will actually cause
> > > all the `needsImplicitFootor` flags to get resolved? And so also I guess
> > > I should never have been looking at those flags directly; I should have
> > > handled this case by calling `LookupSpecialMember` like I do on line
> > > 6196. Is that basically correct?
> > No, not the 6179 one, but the one before it 6163. Yeah you could have :)
> Sorry for the noise, that isn't it because of the if statement right before
> 6163 :/. I was wrong...
>
> Every implicit constructor is already defined before the call to
> `CheckCompletedCXXClass` (except if it's a template), so
> `needsImplicitFootor` is always `false`. This means that you can drop the if
> statement right before 6163, because it's always true.
>
> I'm 99% sure of the previous paragraph. :)
I notice that `test/SemaCXX/implicit-member-functions.cpp` has started failing
in my branch, although there's a *possibility* that that's due to a change in
master. I'm going to investigate a little bit.
Repository:
rC Clang
https://reviews.llvm.org/D50119
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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, in which a copy is made.
The user probably expected a move, but they're not getting a move, perhaps
because the type of "x" is different from the return type of the function. A
place where this comes up in the wild is `stdext::inplace_function`
which implements conversion via a conversion operator rather than a converting
constructor; see https://github.com/WG21-SG14/SG14/issues/125#issue-297201412
Another place where this has come up in the wild, but where the fix ended up
being different, was
try { ... } catch (ExceptionType ex) {
throw ex;
}
where the appropriate fix in that case was to replace `throw ex;` with
`throw;`, and incidentally to catch by reference instead of by value. (But one
could contrive a scenario where the slicing was intentional, in which case
throw-by-move would have been the appropriate fix after all.)
**-Wreturn-std-move-in-c++11**
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do*
produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead.
This is useful in codebases which care about portability to those older
compilers.
The name "-in-c++11" is not technically correct; what caused the
version-to-version change in behavior here was actually CWG 1579, not C++14. So
I'm using the CWG issue number in the diagnostic but "C++11" in the name of the
option. I think it's likely that codebases that need portability to GCC
4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers."
**Discussion**
I fully expect there to be lots of discussion on this patch, and for my first
draft to be missing lots of pieces. I do hope that we can hammer it into a
shape suitable for upstreaming, but milestone number 1 is just to get it out
there for people to use on their own codebases.
Notice that this patch is kind of a belated negative-space version of Richard
Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return
std::move(x)` that should be `return x` for speed. These diagnostics warn about
cases of `return x` that should be `return std::move(x)` for speed. (The two
diagnostics' bailiwicks do not overlap.)
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,262 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+}
+}
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialI
[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;
rsmith wrote:
> Can we say something like "local variable '%0' will be copied despite being
> %select{returned|thrown}1 by name; call 'std::move' explicitly to avoid the
> copy"? (Would that be accurate, given the implementation of the warning?)
>
> Ideally, we'd move the "call 'std::move' explicitly" hint to a separate note
> diagnostic and include a FixItHint on that diagnostic to insert the call to
> `std::move`.
SGTM, except that I don't know how to find out whether we're in the context of
a `return` or a `throw` from this deep in the guts of Sema. Could someone give
me a pointer on that?
I also had trouble figuring out how to generate a fixit from `x` to
`std::move(x)` — it keeps coming out as `std::move( )` — but I expect I'll be
able to solve that one even without help by banging my head against it a bit.
Comment at: include/clang/Sema/Sema.h:3827
bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible);
+ CopyElisionSemanticsKind CESK);
Q: Is it appropriate for me to be changing the signature of this public method
at all? I don't know libclang's rules regarding this kind of change.
Comment at: lib/Sema/SemaExprCXX.cpp:731
if (IsThrownVarInScope)
- NRVOVariable = getCopyElisionCandidate(QualType(), Ex, false);
+ NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict);
Q: I'm not sure what concept is being represented by "CES_Strict" here. If
there is a more standardese-appropriate name, I'd like to use it. And if there
are multiple concepts here that just happen to be identical *coincidentally*,
I'd probably prefer to use multiple enumerators that all just happen to have
value `0`. But because there are so many call-sites and because this is not
directly related to my patch, I have not investigated much.
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] 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
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,262 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast(d); }
+ConvertFromBase ok6() { Derived d; return static_cast(d); }
+
+// If the target is an lvalue reference, assume it's not safe to move from.
+Derived ok_lvalue1(Derived& d) { return d; }
+Base ok_lvalue2(Derived& d) { return d; }
+ConstructFromDerived ok_lvalue3(Derived& d) { return d; }
+ConstructFromBase ok_lvalue4(Derived& d) { return d; }
+ConvertFromDerived ok_lvalue5(Derived& d) { return d; }
+ConvertFromBase ok_lvalue6(Derived& d) { return d; }
+
+// If the target is a global, assume it's not safe to move from.
+static Derived global_d;
+Derived ok_global1() { return global_d; }
+Base ok_global2() { return global_d; }
+ConstructFromDerived ok_
[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
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,311 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast(d); }
+ConvertFromBase ok6() { Derived d; return static_c
[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
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,327 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast(d); }
+ConvertFromBase ok6() { Derived d; return st
[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 minor nit that I don't know how to fix: warn-return-std-move.cpp:220:12: warning: local variable 'd' will be copied despite being returned by name [-Wreturn-std-move] return (d); // e17 ^ warn-return-std-move.cpp:220:12: note: call 'std::move' explicitly to avoid copying return (d); // e17 ^~~ std::move(d) The warning places a caret directly under the `(`, when I wish it would place `^~~` under the entire expression, the way the fixit does. I also spent a little time looking into whether I could/should diagnose auto [x, y] = std::make_pair(Derived(), Derived()); return x; // 'x' will be copied despite being returned by name but I have decided that this is probably too far afield to be rolled into this patch, even if I could figure out how to detect it, which to a first approximation I cannot. So I am deliberately punting on that one. 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] 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 type
originally.) I've verified that the check is still needed (not redundant) along
the other codepath.
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Deri
[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:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static
[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 return take(std::move(mysharedptr)); (Notice that the "return" context is relatively important, because if `mysharedptr` is used again after this use, then the move-from is unsafe and should not be suggested. Of course it's always possible for `mysharedptr` to be used again after a `return` or `throw` as well, e.g. during stack unwinding/destructors; but there's precedent there for C++ saying we'll take that chance.) If you start treating names as rvalues no matter where they appear in larger expressions, then you run into trouble with std::string hello = ...; std::string &hello2 = hello; return concatenate(hello, hello2); where the first use of `hello` on this line moves-out-of it, and then the second use reads the moved-from value. My best guess is that things like that occur frequently enough in real-world code that (A) the Standard can't specify move behavior there because it would quietly break code, and (B) a Clang diagnostic would produce mostly false positives (in the sense that accepting the fixits would quietly break the user's code). I admit it would be interesting to write the code and find out empirically. Whereas in this specific limited case of `return x;` and `throw x;`, programmers are already trained to expect a move, and there are no (new) aliasing issues. > Where though, as clang-tidy check? My experience with this patch suggests that when the thing you need to do involves "run a hypothetical overload-resolution," it is basically impossible to do with clang-tidy, and relatively easy to do in clang proper. (I tried writing this check into clang-tidy first, and couldn't figure out how to do the overload resolution part. Whereas it was painless in clang, especially with `-Wpessimizing-move` to copy off of.) 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] 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] 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, nacl_secure_random, arc4random, rand_s,... If you're on a target that doesn't have any of these, then IMO the appropriate patch would be one of - #ifdef _LIBCPP_USING_YOURPLATFORM_RANDOM - #ifdef _LIBCPP_USING_ALWAYSZERO_RANDOM where the latter provides a "random_device" that yields nothing but zeroes. That would still be very slightly better than providing a non-conforming implementation. (And notice that the useless `double random_device::entropy()` method will for the first time return a *mathematically correct* value for the always-zero random_device! ;)) Repository: rCXX libc++ https://reviews.llvm.org/D41316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 confident that constructing an instance of `random_device` MUST NOT actually call `srand`. (I'd like to say that it shouldn't even call `rand`.) Either of those calls would be observable by the programmer. But there is a precedent for e.g. `random_shuffle` making calls to `rand`. Repository: rCXX libc++ https://reviews.llvm.org/D41316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 it be more appropriate to say
something like "value of type int8_t will be printed as character, not number"?
I had to go all the way down to the test cases in this patch before it occurred
to me what the actual problem being diagnosed here was.
And speaking of "printed": do you care about streaming *in* values of these
types with ">>"?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41740
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone created this revision.
Quuxplusone added a reviewer: rtrieu.
Herald added a subscriber: cfe-commits.
This patch is extracted from https://reviews.llvm.org/D43322, which adds a new
diagnostic `-Wreturn-std-move`. This patch here is just the non-functional
parts of that patch. It pulls `TryMoveInitialization` out into a separate
function so that we can (in the next patch) try it twice — once with current
C++ rules, and once with the rules as they would be if `return x` considered
rvalue-ref-qualified conversion operators. This patch here does *not* add those
new rules; it merely rearranges the existing code to make the next patch less
bulky.
Repository:
rC Clang
https://reviews.llvm.org/D43898
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -741,7 +741,7 @@
if (D->isNRVOVariable()) {
QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, false))
+if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
Var->setNRVOVariable(true);
}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2862,16 +2862,16 @@
/// \param E The expression being returned from the function or block, or
/// being thrown.
///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
/// id-expressions that could be moved out of the function to be considered NRVO
/// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
/// determine whether we should try to move as part of a return or throw (which
/// does allow function parameters).
///
/// \returns The NRVO candidate variable, if the return statement may use the
/// NRVO, or NULL if there is no such candidate.
VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
if (!getLangOpts().CPlusPlus)
return nullptr;
@@ -2884,29 +2884,29 @@
if (!VD)
return nullptr;
- if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+ if (isCopyElisionCandidate(ReturnType, VD, CESK))
return VD;
return nullptr;
}
bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
QualType VDType = VD->getType();
// - in a return statement in a function with ...
// ... a class return type ...
if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
if (!ReturnType->isRecordType())
return false;
// ... the same cv-unqualified type as the function return type ...
// When considering moving this expression out, allow dissimilar types.
-if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
!Context.hasSameUnqualifiedType(ReturnType, VDType))
return false;
}
// ...object (other than a function or catch-clause parameter)...
if (VD->getKind() != Decl::Var &&
- !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+ !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
return false;
if (VD->isExceptionVariable()) return false;
@@ -2918,7 +2918,7 @@
// variable will no longer be used.
if (VD->hasAttr()) return false;
- if (AllowParamOrMoveConstructible)
+ if (CESK & CES_AllowDifferentTypes)
return true;
// ...non-volatile...
@@ -2933,6 +2933,58 @@
return true;
}
+static void TryMoveInitialization(Sema& S,
+ const InitializedEntity &Entity,
+ const VarDecl *NRVOCandidate,
+ QualType ResultType,
+ Expr *Value,
+ ExprResult &Res)
+{
+ ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
+CK_NoOp, Value, VK_XValue);
+
+ Expr *InitExpr = &AsRvalue;
+
+ InitializationKind Kind = InitializationKind::CreateCopy(
+ Value->getLocStart(), Value->getLocStart());
+
+ InitializationSequence Seq(S, Entity, Kind, InitExpr);
+ if (Seq) {
+for (const InitializationSequence::Step &Step : Seq.steps()) {
+ if (!(Step.Kind == InitializationSequence::SK_ConstructorInitialization ||
+Step.Kind == Initia
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone updated this revision to Diff 136380.
Quuxplusone added a comment.
Add a block comment for function `TryMoveInitialization`.
Repository:
rC Clang
https://reviews.llvm.org/D43898
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -741,7 +741,7 @@
if (D->isNRVOVariable()) {
QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, false))
+if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
Var->setNRVOVariable(true);
}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2862,16 +2862,16 @@
/// \param E The expression being returned from the function or block, or
/// being thrown.
///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
/// id-expressions that could be moved out of the function to be considered NRVO
/// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
/// determine whether we should try to move as part of a return or throw (which
/// does allow function parameters).
///
/// \returns The NRVO candidate variable, if the return statement may use the
/// NRVO, or NULL if there is no such candidate.
VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
if (!getLangOpts().CPlusPlus)
return nullptr;
@@ -2884,29 +2884,29 @@
if (!VD)
return nullptr;
- if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+ if (isCopyElisionCandidate(ReturnType, VD, CESK))
return VD;
return nullptr;
}
bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
QualType VDType = VD->getType();
// - in a return statement in a function with ...
// ... a class return type ...
if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
if (!ReturnType->isRecordType())
return false;
// ... the same cv-unqualified type as the function return type ...
// When considering moving this expression out, allow dissimilar types.
-if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
!Context.hasSameUnqualifiedType(ReturnType, VDType))
return false;
}
// ...object (other than a function or catch-clause parameter)...
if (VD->getKind() != Decl::Var &&
- !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+ !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
return false;
if (VD->isExceptionVariable()) return false;
@@ -2918,7 +2918,7 @@
// variable will no longer be used.
if (VD->hasAttr()) return false;
- if (AllowParamOrMoveConstructible)
+ if (CESK & CES_AllowDifferentTypes)
return true;
// ...non-volatile...
@@ -2933,6 +2933,70 @@
return true;
}
+/// \brief Try to perform the initialization of a potentially-movable value,
+/// which is the operand to a return or throw statement.
+///
+/// This routine implements C++14 [class.copy]p32, which attempts to treat
+/// returned lvalues as rvalues in certain cases (to prefer move construction),
+/// then falls back to treating them as lvalues if that failed.
+///
+/// \param Res We will fill this in if move-initialization was possible.
+/// If move-initialization is not possible, such that we must fall back to
+/// treating the operand as an lvalue, we will leave Res in its original
+/// invalid state.
+static void TryMoveInitialization(Sema& S,
+ const InitializedEntity &Entity,
+ const VarDecl *NRVOCandidate,
+ QualType ResultType,
+ Expr *Value,
+ ExprResult &Res)
+{
+ ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
+CK_NoOp, Value, VK_XValue);
+
+ Expr *InitExpr = &AsRvalue;
+
+ InitializationKind Kind = InitializationKind::CreateCopy(
+ Value->getLocStart(), Value->getLocStart());
+
+ InitializationSequence Seq(S, Entity, Kind, InitExpr);
+ if (Seq) {
+for (const InitializationSequence::Step &Step : Seq.steps()) {
+ if (!(Step.Kind == Initia
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added inline comments. Comment at: lib/Sema/SemaStmt.cpp:2937 +static void AttemptMoveInitialization(Sema& S, + const InitializedEntity &Entity, rtrieu wrote: > I have a few concerns about this function. The code isn't a straight move > from the old location to this one, so it is hard to follow along the changes, > especially the change to the complex if conditional. The function should be > commented, especially to behavior difference for setting IsFake. > > It looks like when IsFake is set, then the result is discarded and not used, > but it is still possible on success for AsRvalue to be copied to the heap. > This is wasteful when it is never used again. > > Another issue is the Value in the original code is used again towards the end > of the function on line #3013. In the old code, Value can be updated while > in the new code, it does. > > It may be better to split this change in two, the first adding this function > and the CopyElisionSemanticsKind enum and the second adding the diagnostic > itself. Hi @rtrieu, and thanks! I have split out the first half of the patch into a new revision D43898, and updated this one with the full patch (both halves together). Is there an easy way for me to make "just the second half" reviewable on its own, before the first half has been merged to master? > It looks like when IsFake is set, then the result is discarded and not used, > but it is still possible on success for AsRvalue to be copied to the heap. > This is wasteful when it is never used again. I believe you are correct. But I'm not sure if it's safe to use `AsRvalue` as input to `Res` (which *is* used outside this function) if it's not moved like this; I don't know much about the internals here. You or anyone have a suggestion for how to fix that issue? > In the old code, Value can be updated while in the new code, it does. I can't parse this, sorry. FYI, in the patch I'm about to upload, I have renamed `!IsFake` to `ConvertingConstructorsOnly`, which should be more pleasing to the eye. :) 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] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone updated this revision to Diff 136383.
Quuxplusone added a comment.
Rename some functions and parameters. Rebase onto
https://reviews.llvm.org/D43898.
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() { Derived d; retu
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5639
+ "local variable %0 will be copied despite being %select{returned|thrown}1 by
name">,
+ InGroup, DefaultIgnore;
+def note_add_std_move : Note<
I would like some guidance on whether it would be appropriate to turn this
warning on as part of `-Wmove`.
Comment at: lib/Sema/SemaStmt.cpp:3083
+Diag(Value->getExprLoc(), diag::warn_return_std_move_in_cxx11)
+<< Value->getSourceRange()
+<< NRVOCandidate->getDeclName() << ResultType << QT;
This source range does the right thing; thanks @rtrieu!
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] D43766: [clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique
Quuxplusone added inline comments.
Comment at: clang-tidy/modernize/MakeUniqueCheck.cpp:21
+: MakeSmartPtrCheck(Name, Context, "std::make_unique"),
+ MinimumLanguageVersion(Options.get("MakeUniqueLanguageVersion",
+ getDefaultMinimumLanguageVersion()))
{}
aaron.ballman wrote:
> alexfh wrote:
> > ftingaud wrote:
> > > aaron.ballman wrote:
> > > > Why is this is a user-facing option?
> > > >
> > > > If it needs to be a user-facing option, you also need to implement an
> > > > override for `storeOptions()` as well.
> > > As the rule was very customizable, I also made the c++ version
> > > customizable. But the option is only useful if a user has a custom
> > > make_unique that needs c++14 (uses std::make_unique internally) and
> > > multiple versions of C++ in their codeline. I agree this is a rather
> > > specific usecase. I can remove it and make the code simpler if it is your
> > > recommendation.
> > > if a user has a custom make_unique that needs c++14
> >
> > The only reason to have a custom make_unique that I know is to workaround
> > the absence of std::make_unique in c++11. So this looks like a not very
> > useful case to support.
> Agreed, I'd drop it.
From the peanut gallery: IIUC, yes, this is not a useful case to support.
If the user has a custom `my::make_unique`, then it is *by definition* usable
in C++11. Now, it might still be implemented as a call to `std::make_unique` in
C++14... but it will not *require* C++14.
The user's `my::make_unique` will likely require C++11 (not just C++03), but I
personally don't think clang-tidy ought to cater to corner cases that involve
C++03. I'd just assume that an instruction to "use `my::make_unique`" is
appropriate for any translation unit that's being tidied with a custom
make_unique.
Comment at: test/clang-tidy/modernize-make-unique-cxx14.cpp:10
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use std::make_unique instead
+ // CHECK-FIXES: auto my_ptr = std::make_unique(1);
+ return 0;
IIUC above, you allow the user to pass in the name of a custom
`my::make_unique` function. Could you add a test case for that?
https://reviews.llvm.org/D43766
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone added a comment. @rtrieu please take a look? Also, I would need someone to commit this for me, as I don't have commit privs. Repository: rC Clang https://reviews.llvm.org/D43898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41316: [libcxx] Allow random_device to be built optionally
Quuxplusone added inline comments. Comment at: test/std/numerics/rand/rand.device/lit.local.cfg:1 +# Disable all of the random device tests if the correct feature is not available. +if 'libcpp-has-no-random-device' in config.available_features: EricWF wrote: > There are only 3 tests under this directory. I would rather mark each one > explicitly with `// UNSUPPORTED: libcpp-has-no-random-device` @weimingz: looks like this request from Eric is unresolved. FWIW: I'm still ambivalent about the whole direction of this patch; but I'm happy if everyone else is happy. I don't anticipate any new comments from me. https://reviews.llvm.org/D41316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added a comment. @rtrieu (and perhaps @rsmith) ping? The action items I need help with are: - Review and land the refactoring patch https://reviews.llvm.org/D43898 (I don't have commit privs) - Ideally, test compiling a bunch of (e.g. Google) code with https://reviews.llvm.org/D43322, see if there are any rough edges - Decide whether `-Wmove` should imply `-Wreturn-std-move` - Review and land the new-diagnostic patch https://reviews.llvm.org/D43322 - Ideally, some discussion of whether I should write a paper for San Diego proposing that C++ *should* move rather than copy in these cases, and whether that possibility should change anything about this patch Speaking of rough edges, I did find that in my employer's codebase the far-and-away-most-common trigger is `return my_shared_ptr_to_derived_class;` in a function that is declared to return `shared_ptr`. This does indeed generate an extra atomic increment/decrement compared to the optimal code, but I could imagine someone considering that a "false positive." Other than that one questionable scenario, everything else I've found with it so far has been a true positive. There are no positives (either true or false) in LLVM's codebase. 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] D44143: Create properly seeded random generator check
Quuxplusone added inline comments. Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:26 +std::random_device dev; +std::mt19937 engine3(dev()); // Good + } Seeding MT19937 with a single 32-bit integer is //not// "Good". It makes the seed super easy to brute-force; and for example, `engine3` will never produce 7 or 13 as its first output. http://www.pcg-random.org/posts/cpp-seeding-surprises.html This doesn't affect the implementation or usefulness of this clang-tidy check, which is pretty nifty. I merely object to marking this sample code with the comment "Good" in official documentation. It should be marked "Will not warn" at best. Or replace it with something slightly more realistic, e.g. int x = atoi(argv[1]); std::mt19937 engine3(x); // Will not warn As Aaron said above, seeding with the current time is approximately as good an idea, and "will not warn" with the current diagnostic either. The correct way to seed a PRNG is to initialize the //entire state// with random bits, not just 32 bits of the state. This can be done, but not yet in standard C++: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0205r0.html Comment at: test/clang-tidy/cert-properly-seeded-random-generator.cpp:76 + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator] + engine1.seed(seed); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator] Is the diagnostic suppressed if `seed` is a template parameter? (Not that I'd do this. It's just a corner case I thought of.) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44143 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added a comment. Btw, I'm going to be talking about this patch tonight at 7 in Palo Alto. :) https://www.meetup.com/ACCU-Bay-Area/events/248040207/ https://docs.google.com/presentation/d/18ZRnedocXSQKn9Eh67gGv-ignReHfRD7vj_dxrra1kc/ 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] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone added a comment. @rtrieu ping? Repository: rC Clang https://reviews.llvm.org/D43898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone marked 4 inline comments as done.
Quuxplusone added inline comments.
Comment at: lib/Sema/SemaStmt.cpp:2970
+ FunctionDecl *FD = Step.Function.Function;
+ if (isa(FD)) {
+// C++14 [class.copy]p32:
rtrieu wrote:
> Use early exit here:
>
>
> ```
> if (!isa(FD)
> continue;
>
> // old if-block code
> ```
I'd prefer not to do this, since D43322 is going to change this code into "if
isa-red-fish... else if isa-blue-fish...". Therefore I think it makes sense to
keep this intermediate stage as "if isa-red-fish...", rather than changing it
into "if not-isa-red-fish continue... otherwise..."
If you really want this, I can change it; but it's just going to change back in
D43322, and the goal of this patch was to make D43322 smaller.
Comment at: lib/Sema/SemaStmt.cpp:2999-3000
-// expression node to persist.
-Value = ImplicitCastExpr::Create(Context, Value->getType(), CK_NoOp,
- Value, nullptr, VK_XValue);
rtrieu wrote:
> At this point, the variable Value is updated. Value is scoped to this
> function, and used again after this code. In your change, there is a new
> Value variable in the static function. Only that variable is updated and not
> this one, making this a change in behavior.
Good catch! I've addressed this now by making the parameter `Expr *&Value`; but
I'd be open to better approaches. Particularly because I still don't know what
to do about the "unnecessary promoting `Value` to the heap" that will happen in
D43322.
Repository:
rC Clang
https://reviews.llvm.org/D43898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone updated this revision to Diff 137920.
Quuxplusone marked an inline comment as done.
Quuxplusone added a comment.
Addressed @rtrieu's comments.
@rtrieu, please take another look at this and https://reviews.llvm.org/D43322?
Thanks!
Repository:
rC Clang
https://reviews.llvm.org/D43898
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -741,7 +741,7 @@
if (D->isNRVOVariable()) {
QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, false))
+if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
Var->setNRVOVariable(true);
}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2862,16 +2862,16 @@
/// \param E The expression being returned from the function or block, or
/// being thrown.
///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
/// id-expressions that could be moved out of the function to be considered NRVO
/// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
/// determine whether we should try to move as part of a return or throw (which
/// does allow function parameters).
///
/// \returns The NRVO candidate variable, if the return statement may use the
/// NRVO, or NULL if there is no such candidate.
VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
if (!getLangOpts().CPlusPlus)
return nullptr;
@@ -2884,29 +2884,29 @@
if (!VD)
return nullptr;
- if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+ if (isCopyElisionCandidate(ReturnType, VD, CESK))
return VD;
return nullptr;
}
bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
QualType VDType = VD->getType();
// - in a return statement in a function with ...
// ... a class return type ...
if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
if (!ReturnType->isRecordType())
return false;
// ... the same cv-unqualified type as the function return type ...
// When considering moving this expression out, allow dissimilar types.
-if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
!Context.hasSameUnqualifiedType(ReturnType, VDType))
return false;
}
// ...object (other than a function or catch-clause parameter)...
if (VD->getKind() != Decl::Var &&
- !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+ !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
return false;
if (VD->isExceptionVariable()) return false;
@@ -2918,7 +2918,7 @@
// variable will no longer be used.
if (VD->hasAttr()) return false;
- if (AllowParamOrMoveConstructible)
+ if (CESK & CES_AllowDifferentTypes)
return true;
// ...non-volatile...
@@ -2933,6 +2933,71 @@
return true;
}
+/// \brief Try to perform the initialization of a potentially-movable value,
+/// which is the operand to a return or throw statement.
+///
+/// This routine implements C++14 [class.copy]p32, which attempts to treat
+/// returned lvalues as rvalues in certain cases (to prefer move construction),
+/// then falls back to treating them as lvalues if that failed.
+///
+/// \param Res We will fill this in if move-initialization was possible.
+/// If move-initialization is not possible, such that we must fall back to
+/// treating the operand as an lvalue, we will leave Res in its original
+/// invalid state.
+static void TryMoveInitialization(Sema& S,
+ const InitializedEntity &Entity,
+ const VarDecl *NRVOCandidate,
+ QualType ResultType,
+ Expr *&Value,
+ ExprResult &Res) {
+ ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
+CK_NoOp, Value, VK_XValue);
+
+ Expr *InitExpr = &AsRvalue;
+
+ InitializationKind Kind = InitializationKind::CreateCopy(
+ Value->getLocStart(), Value->getLocStart());
+
+ InitializationSequence Seq(S, Entity, Kind, InitExpr);
+
+ if
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone updated this revision to Diff 137921.
Quuxplusone added a comment.
Rebased on https://reviews.llvm.org/D43898 after addressing @rtrieu's latest
comments over there.
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() {
[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.
Quuxplusone added a comment. @rtrieu thanks! I don't have commit privileges; could I ask you to commit this on my behalf? (Or if not, please say no, and I'll know to go looking for someone else to ask.) Once/if https://reviews.llvm.org/D43898 hits master, I'll rebase https://reviews.llvm.org/D43322 and await a LGTM there, at which point I'll again ask for help committing it. :) Repository: rC Clang https://reviews.llvm.org/D43898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone updated this revision to Diff 138457.
Quuxplusone added a comment.
Boldly add `-Wreturn-std-move` to `-Wmove` (and thus also to `-Wall`).
The backward-compatibility-concerned diagnostic, `-Wreturn-std-move-in-c++11`,
is *not* appropriate for `-Wmove`; but I believe this main diagnostic is
appropriate.
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDeri
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:388 def PessimizingMove : DiagGroup<"pessimizing-move">; +def ReturnStdMoveInCXX11 : DiagGroup<"return-std-move-in-c++11">; +def ReturnStdMove : DiagGroup<"return-std-move">; >> The backward-compatibility-concerned diagnostic, -Wreturn-std-move-in-c++11, >> is *not* appropriate for -Wmove; > Have you evaluated possibility of adding -Wreturn-std-move-in-c++11 to one of > CXX..Compat groups? Hmm. I've considered it, but I don't know what the appropriate action is... I suspect that it should *not* be "CXX11Compat" because technically the change was a DR against C++11 — sadly this is about portability to "older" compilers, not portability to "lesser" C++ standards. Of course it can't be "CXX98Compat", for several reasons. :) I'm not sure of the purpose of the "CXX..CompatPedantic" groups. They are all identical to the "CXX..Compat" groups, except that CXX98CompatPedantic includes one extra warning about binding to lifetime-extended temporaries which don't have copy constructors. 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] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone updated this revision to Diff 138594.
Quuxplusone added a comment.
Rebase on master, now that the refactoring https://reviews.llvm.org/D43898 has
been pushed (thanks @rtrieu!)
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaStmt.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+template struct remove_reference { typedef T type; };
+
+template typename remove_reference::type &&move(T &&t);
+} // namespace foo
+} // namespace std
+
+struct Instrument {
+Instrument() {}
+Instrument(Instrument&&) { /* MOVE */ }
+Instrument(const Instrument&) { /* COPY */ }
+};
+struct ConvertFromBase { Instrument i; };
+struct ConvertFromDerived { Instrument i; };
+struct Base {
+Instrument i;
+operator ConvertFromBase() const& { return ConvertFromBase{i}; }
+operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; }
+};
+struct Derived : public Base {
+operator ConvertFromDerived() const& { return ConvertFromDerived{i}; }
+operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; }
+};
+struct ConstructFromBase {
+Instrument i;
+ConstructFromBase(const Base& b): i(b.i) {}
+ConstructFromBase(Base&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromDerived {
+Instrument i;
+ConstructFromDerived(const Derived& d): i(d.i) {}
+ConstructFromDerived(Derived&& d): i(std::move(d.i)) {}
+};
+
+struct TrivialInstrument {
+int i = 42;
+};
+struct ConvertFromTrivialBase { TrivialInstrument i; };
+struct ConvertFromTrivialDerived { TrivialInstrument i; };
+struct TrivialBase {
+TrivialInstrument i;
+operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; }
+operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; }
+};
+struct TrivialDerived : public TrivialBase {
+operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; }
+operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; }
+};
+struct ConstructFromTrivialBase {
+TrivialInstrument i;
+ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {}
+ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {}
+};
+struct ConstructFromTrivialDerived {
+TrivialInstrument i;
+ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {}
+ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {}
+};
+
+Derived test1() {
+Derived d1;
+return d1; // ok
+}
+Base test2() {
+Derived d2;
+return d2; // e1
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)"
+}
+ConstructFromDerived test3() {
+Derived d3;
+return d3; // e2-cxx11
+// expected-warning@-1{{would have been copied despite being returned by name}}
+// expected-note@-2{{to avoid copying on older compilers}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)"
+}
+ConstructFromBase test4() {
+Derived d4;
+return d4; // e3
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)"
+}
+ConvertFromDerived test5() {
+Derived d5;
+return d5; // e4
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)"
+}
+ConvertFromBase test6() {
+Derived d6;
+return d6; // e5
+// expected-warning@-1{{will be copied despite being returned by name}}
+// expected-note@-2{{to avoid copying}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)"
+}
+
+// These test cases should not produce the warning.
+Derived ok1() { Derived d; return d; }
+Base ok2() { Derived d; return static_cast(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast(d); }
+ConstructFromBase ok4() { Derived d; return static_cast(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast(d); }
+ConvertFromBase ok6()
[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency
Quuxplusone added a comment. @rtrieu ping! I've rebased this on the pure refactoring you committed for me last week. 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] 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) == 32`.
Repository:
rCXX libc++
https://reviews.llvm.org/D47358
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.ctor/ctor_does_not_allocate.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.ctor/sync_with_default_resource.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.ctor/unsync_with_default_resource.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/equality.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/sync_allocate.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/sync_allocate_overaligned_request.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/sync_allocate_reuse_blocks.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/unsync_allocate.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/unsync_allocate_overaligned_request.pass.cpp
test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/unsync_allocate_reuse_blocks.pass.cpp
test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp
test/support/count_new.hpp
Index: test/support/count_new.hpp
===
--- test/support/count_new.hpp
+++ test/support/count_new.hpp
@@ -211,6 +211,11 @@
return disable_checking || n != delete_called;
}
+bool checkDeleteCalledGreaterThan(int n) const
+{
+return disable_checking || delete_called > n;
+}
+
bool checkAlignedNewCalledEq(int n) const
{
return disable_checking || n == aligned_new_called;
Index: test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.pool/pool_options.pass.cpp
@@ -0,0 +1,25 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// UNSUPPORTED: apple-clang-7
+
+//
+
+// struct pool_options
+
+#include
+#include
+
+int main()
+{
+const std::experimental::pmr::pool_options p;
+assert(p.max_blocks_per_chunk == 0);
+assert(p.largest_required_pool_block == 0);
+}
Index: test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/unsync_allocate_reuse_blocks.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.pool/memory.resource.pool.mem/unsync_allocate_reuse_blocks.pass.cpp
@@ -0,0 +1,53 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class unsynchronized_pool_resource
+
+#include
+#include
+
+#include "count_new.hpp"
+
+static bool is_aligned_to(void *p, size_t alignment)
+{
+void *p2 = p;
+size_t space = 1;
+void *result = std::align(alignment, 1, p2, space);
+return (result == p);
+}
+
+int main()
+{
+globalMemCounter.reset();
+std::experimental::pmr::pool_options opts { 1, 256 };
+std::experimental::pmr::unsynchronized_pool_resource unsync1(opts, std::experimental::pmr::new_delete_resource());
+std::experimental::pmr::memory_resource & r1 = unsync1;
+
+void *ret = r1.allocate(8);
+assert(ret != nullptr);
+assert(is_aligned_to(ret, 8));
+assert(globalMemCounter.checkNewCalledGreaterThan(0));
+int new_called = globalMemCounter.new_called;
+
+// After deallocation, the pool for 8-byte blocks should have at least one vacancy.
+r1.deallocate(ret, 8);
+assert(globalMemCounter.new_called == new_called);
+assert(globalMemCounter.checkDeleteCalledEq(0));
+
+// This should return an existing block from the pool: no new allocations.
+ret = r1.allocate(8);
+assert(ret != nullptr);
+assert(is_aligned_to(ret, 8));
+
[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++
https://reviews.llvm.org/D46806
Files:
include/__functional_base
include/experimental/memory_resource
Index: include/experimental/memory_resource
===
--- include/experimental/memory_resource
+++ include/experimental/memory_resource
@@ -71,7 +71,7 @@
#include
#include
#include
-#include
+#include <__tuple>
#include
#include
#include
@@ -96,7 +96,7 @@
}
// 8.5, memory.resource
-class _LIBCPP_TEMPLATE_VIS memory_resource
+class _LIBCPP_TYPE_VIS memory_resource
{
static const size_t __max_align = alignof(max_align_t);
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args
&&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
Index: include/experimental/memory_resource
===
--- include/experimental/memory_resource
+++ include/experimental/memory_resource
@@ -71,7 +71,7 @@
#include
#include
#include
-#include
+#include <__tuple>
#include
#include
#include
@@ -96,7 +96,7 @@
}
// 8.5, memory.resource
-class _LIBCPP_TEMPLATE_VIS memory_resource
+class _LIBCPP_TYPE_VIS memory_resource
{
static const size_t __max_align = alignof(max_align_t);
Index: include/__functional_base
===
--- include/__functional_base
+++ include/__functional_base
@@ -646,16 +646,6 @@
new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
}
-// FIXME: Theis should have a version which takes a non-const alloc.
-template
-inline _LIBCPP_INLINE_VISIBILITY
-void __user_alloc_construct (_Tp *__storage, const _Allocator &__a, _Args &&... __args)
-{
-__user_alloc_construct_impl(
- __uses_alloc_ctor<_Tp, _Allocator>(),
- __storage, __a, _VSTD::forward<_Args>(__args)...
-);
-}
#endif // _LIBCPP_CXX03_LANG
_LIBCPP_END_NAMESPACE_STD
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 tests in this patch reflect an old, out-of-date snapshot of what had been in those patches a couple weeks ago. Comment at: include/__memory_resource_base:53 +#include +#include +#include This should be `#include <__tuple>` Comment at: include/memory_resource:47 +#include +#include +#include This line should be gone. Repository: rCXX libc++ https://reviews.llvm.org/D47360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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()`. If that's legal, then we can save 8 bytes here. I've asked for an LWG issue to be opened on the subject of "what the heck is `release()` supposed to do anyway." Repository: rCXX libc++ https://reviews.llvm.org/D47111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 thought this was required by the Standard, and maybe it is, but that
was before I discovered this terrible effect, and before Pablo Halpern told me
that it was not his actual intent.
Since the only purpose of storing `__next_buffer_size_` in the object was to
implement the "looping blows up" misfeature, we can now get rid of that field,
which shrinks `monotonic_buffer_resource` from 56 bytes down to 48 bytes,
matching Boost.Container's implementation.
- Add `REQUIRES: c++experimental` to all tests (oops).
Repository:
rCXX libc++
https://reviews.llvm.org/D47111
Files:
include/experimental/memory_resource
src/experimental/memory_resource.cpp
test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/copy_move.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/with_default_resource.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/without_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_initial_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_zero_sized_buffer.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_with_initial_size.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp
@@ -0,0 +1,13 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
@@ -0,0 +1,62 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// REQUIRES: c++experimental
+// UNSUPPORTED: c++98, c++03
+
+//
+
+// class monotonic_buffer_resource
+
+#include
+#include
+#include
+#include
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+virtual void * do_allocate(size_t, size_t)
+{ assert(false); }
+
+virtual void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+virtual bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+int main()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+
