[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372361: [Consumed] Treat by-value class arguments as 
consuming by default, like rvalue… (authored by comex, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67743?vs=220778=220916#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67743/new/

https://reviews.llvm.org/D67743

Files:
  cfe/trunk/lib/Analysis/Consumed.cpp
  cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp


Index: cfe/trunk/lib/Analysis/Consumed.cpp
===
--- cfe/trunk/lib/Analysis/Consumed.cpp
+++ cfe/trunk/lib/Analysis/Consumed.cpp
@@ -644,10 +644,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))
Index: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,12 +53,18 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
 };
 
+void dtByVal(DestructorTester);
+void dtByValMarkUnconsumed(DestructorTester RETURN_TYPESTATE(unconsumed));
+
 void baf0(const ConsumableClass  var);
 void baf1(const ConsumableClass );
 void baf2(const ConsumableClass *var);
@@ -120,6 +126,19 @@
  expected-warning {{invalid invocation of method 
'~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+// both the var and the temporary are consumed:
+DestructorTester D0(nullptr);
+dtByVal((DestructorTester &&)D0);
+  }
+  {
+// the var is consumed but the temporary isn't:
+DestructorTester D1(nullptr);
+dtByValMarkUnconsumed((DestructorTester &&)D1); // expected-warning 
{{invalid invocation of method '~DestructorTester' on a temporary object while 
it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass(); // expected-warning {{invalid invocation of method 
'operator*' on a temporary object while it is in the 'consumed' state}}
 }
@@ -413,10 +432,15 @@
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass & 
RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass &&)var);
   
   *var;
 }
@@ -480,6 +504,9 @@
   
   baf2();  
   *var;
+
+  baf3(var);
+  *var;
   
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on 
object 'var' while it is in the 'unknown' state}}


Index: cfe/trunk/lib/Analysis/Consumed.cpp
===
--- cfe/trunk/lib/Analysis/Consumed.cpp
+++ cfe/trunk/lib/Analysis/Consumed.cpp
@@ -644,10 +644,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))
Index: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,12 +53,18 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
 };
 
+void 

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

In D67743#1675533 , @dblaikie wrote:

> "Also, fix the order of if statements so that an explicit return_typestate 
> annotation takes precedence over the default behavior for rvalue refs."
>
> I'd probably have split that out into a separate patch - in part to discuss 
> the design implications of that. I have some doubts about supporting 
> non-consumed after passing by rvalue ref, but don't feel too strongly & the 
> alternative would be having a warning about the attribute being ignored, etc 
> - when it has a fairly clear meaning if it is there, just not sure people 
> 'should' be doing that.


Fair enough.  I agree there's not much reason to do that, but it also seems 
pretty harmless to respect the user's choice.  Silently ignoring the attribute 
as before is obviously wrong, so the alternative would be adding a diagnostic 
for that case, but it doesn't seem worth it...

> unless these are testing something noteworthy about them being static 
> functions I'd probably suggest making them non-members like the other test 
> functions below, for consistency (otherwise the inconsistency tends to raise 
> the question of "what is significant about this difference?")

Okay, I'll change them to non-members before committing.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67743/new/

https://reviews.llvm.org/D67743



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

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[David Blaikie via Phabricator via cfe-commits]

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

"Also, fix the order of if statements so that an explicit return_typestate 
annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the 
design implications of that. I have some doubts about supporting non-consumed 
after passing by rvalue ref, but don't feel too strongly & the alternative 
would be having a warning about the attribute being ignored, etc - when it has 
a fairly clear meaning if it is there, just not sure people 'should' be doing 
that.




Comment at: test/SemaCXX/warn-consumed-analysis.cpp:62-64
+
+  static void byVal(DestructorTester);
+  static void byValMarkUnconsumed(DestructorTester 
RETURN_TYPESTATE(unconsumed));

unless these are testing something noteworthy about them being static functions 
I'd probably suggest making them non-members like the other test functions 
below, for consistency (otherwise the inconsistency tends to raise the question 
of "what is significant about this difference?")


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67743/new/

https://reviews.llvm.org/D67743



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

[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also, fix the order of `if` statements so that an explicit `return_typestate` 
annotation takes precedence over the default behavior for rvalue refs.

Note that for by-value arguments, the object being consumed is always a 
temporary.  If you write:

`void a(X); a((X &&)x);`

...this first constructs a temporary X by moving from `x`, then calls `a` with 
a pointer to the temporary.  Before and after this change, the move copies 
`x`'s typestate to the temporary and marks `x` itself as consumed.  But before 
this change, if the temporary started out unconsumed (because `x` was 
unconsumed before the move), it would still be unconsumed when its destructor 
was run after the call.  Now it will be considered consumed at that point.

Also note that currently, only parameters explicitly marked `return_typestate` 
have their state-on-return checked on the callee side.  Parameters which are 
implicitly consuming due to being rvalue references – or, after this patch, 
by-value – are checked only on the caller side.  This discrepancy was very 
surprising to me as a user.  I wrote a fix, but in my codebase it broke some 
code that was using `std::forward`.  Therefore, I plan to hold off on 
submitting the fix until a followup patch, which will generalize the current 
`std::move` special case into an attribute that can be applied to any 'cast' 
function like `std::move` and `std::forward`.


Repository:
  rC Clang

https://reviews.llvm.org/D67743

Files:
  lib/Analysis/Consumed.cpp
  test/SemaCXX/warn-consumed-analysis.cpp


Index: test/SemaCXX/warn-consumed-analysis.cpp
===
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,10 +53,15 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
+  static void byVal(DestructorTester);
+  static void byValMarkUnconsumed(DestructorTester 
RETURN_TYPESTATE(unconsumed));
 };
 
 void baf0(const ConsumableClass  var);
@@ -120,6 +125,19 @@
  expected-warning {{invalid invocation of method 
'~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+// both the var and the temporary are consumed:
+DestructorTester D0(nullptr);
+DestructorTester::byVal((DestructorTester &&)D0);
+  }
+  {
+// the var is consumed but the temporary isn't:
+DestructorTester D1(nullptr);
+DestructorTester::byValMarkUnconsumed((DestructorTester &&)D1); // 
expected-warning {{invalid invocation of method '~DestructorTester' on a 
temporary object while it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass(); // expected-warning {{invalid invocation of method 
'operator*' on a temporary object while it is in the 'consumed' state}}
 }
@@ -413,10 +431,15 @@
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass & 
RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass &&)var);
   
   *var;
 }
@@ -480,6 +503,9 @@
   
   baf2();  
   *var;
+
+  baf3(var);
+  *var;
   
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on 
object 'var' while it is in the 'unknown' state}}
Index: lib/Analysis/Consumed.cpp
===
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -645,10 +645,10 @@
   continue;
 
 // Adjust state on the caller side.
-if (isRValueRef(ParamType))
-  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-else if (ReturnTypestateAttr *RT = Param->getAttr())
+if (ReturnTypestateAttr *RT = Param->getAttr())
   setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+  setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
 else if (isPointerOrRef(ParamType) &&
  (!ParamType->getPointeeType().isConstQualified() ||
   isSetOnReadPtrType(ParamType)))


Index: test/SemaCXX/warn-consumed-analysis.cpp
===
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,10 +53,15 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*()