[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D67647#1713974 , @comex wrote:

> So, I landed this patch but had to revert it as it broke the build on MSVC 
> (and only MSVC).  That was almost a month ago, but I haven't gotten back 
> around to this until now – sorry :|
>
> In this updated patch, I switched is_random_iterator from a constexpr 
> function:
>
>   template 
>   constexpr bool is_random_iterator() {
> return std::is_same<
>   typename std::iterator_traits::iterator_category,
>   std::random_access_iterator_tag>::value;
>   }
>
>
> to a type alias:
>
>   template 
>   using is_random_iterator =
> std::is_same::iterator_category,
>  std::random_access_iterator_tag>;
>
>
> and changed the uses accordingly.  For some reason, MSVC thought the two 
> overloads of `drop_begin`, one with `enable_if()>` and 
> one with `enable_if()>`, were duplicate definitions.  
> But with `is_random_iterator` changed to a type alias, MSVC is fine with 
> them.  GCC and Clang think both versions are valid, so I think it's just an 
> MSVC bug.
>
> Simplified example for reference: https://gcc.godbolt.org/z/niXCy4


Sounds good to me. Maybe leave a comment explaining why a constexpr function 
was not used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 225557.
comex marked 2 inline comments as done.
comex added a comment.

So, I landed this patch but had to revert it as it broke the build on MSVC (and 
only MSVC).  That was almost a month ago, but I haven't gotten back around to 
this until now – sorry :|

In this updated patch, I switched is_random_iterator from a constexpr function:

  template 
  constexpr bool is_random_iterator() {
return std::is_same<
  typename std::iterator_traits::iterator_category,
  std::random_access_iterator_tag>::value;
  }

to a type alias:

  template 
  using is_random_iterator =
std::is_same::iterator_category,
 std::random_access_iterator_tag>;

and changed the uses accordingly.  For some reason, MSVC thought the two 
overloads of `drop_begin`, one with `enable_if()>` and 
one with `enable_if()>`, were duplicate definitions.  But 
with `is_random_iterator` changed to a type alias, MSVC is fine with them.  GCC 
and Clang think both versions are valid, so I think it's just an MSVC bug.

Simplified example for reference: https://gcc.godbolt.org/z/niXCy4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,15 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+using is_random_iterator =
+  std::is_same::iterator_category,
+   std::random_access_iterator_tag>;
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -59,11 +65,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if::value,
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
+}
+
+/// Optimized version for random iterators
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if::value,
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
 }
+
 }
 
 #endif
Index: llvm/include/llvm/ADT/STLExtras.h
===
--- llvm/include/llvm/ADT/STLExtras.h
+++ llvm/include/llvm/ADT/STLExtras.h
@@ -1573,6 +1573,14 @@
 }
 template  constexpr T *to_address(T *P) { return P; }
 
+template 
+auto index(R &&TheRange,
+  typename std::iterator_traits>::difference_type N)
+  -> decltype(TheRange.begin()[N]) {
+  assert(N < TheRange.end() - TheRange.begin() && "Index out of range!");
+  return TheRange.begin()[N];
+}
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STLEXTRAS_H
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argum

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: llvm/include/llvm/ADT/iterator_range.h:27-33
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+

comex wrote:
> dblaikie wrote:
> > Is this how the C++ standard is doing things these days? Or is it usually 
> > done with variable templates?
> "Type trait"-like things have gone through three different evolutions:
> 
> - C++11 struct templates: `std::is_integral::value`
> - C++17 variable templates: `std::is_integral_v`
> - C++20 concepts: `std::integral`
> 
> In fact, the C++20 draft has a `random_access_iterator` concept, though 
> there is no `is_random_access_iterator` in prior versions.
> 
> However, with LLVM still built as C++11, this helper has to be a struct 
> template or a constexpr function.  It seemed a bit simpler to use a constexpr 
> function, since template specialization wasn't needed.
Fair enough - thanks for the details :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex marked 2 inline comments as done.
comex added inline comments.



Comment at: llvm/include/llvm/ADT/iterator_range.h:27-33
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+

dblaikie wrote:
> Is this how the C++ standard is doing things these days? Or is it usually 
> done with variable templates?
"Type trait"-like things have gone through three different evolutions:

- C++11 struct templates: `std::is_integral::value`
- C++17 variable templates: `std::is_integral_v`
- C++20 concepts: `std::integral`

In fact, the C++20 draft has a `random_access_iterator` concept, though 
there is no `is_random_access_iterator` in prior versions.

However, with LLVM still built as C++11, this helper has to be a struct 
template or a constexpr function.  It seemed a bit simpler to use a constexpr 
function, since template specialization wasn't needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

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

Sounds good - thanks!




Comment at: llvm/include/llvm/ADT/iterator_range.h:27-33
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+

Is this how the C++ standard is doing things these days? Or is it usually done 
with variable templates?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220926.
comex added a comment.

In D67647#1674795 , @dblaikie wrote:

> Right - I was suggesting that could be changed. Would it be OK to you to 
> change arguments() to return ArrayRef? Or would you rather avoid that to 
> avoid baking in more dependence on that alias violation?


I'd rather avoid that for the reason you said.

> Same reason I've pushed back on adding things like "empty()" or "size()", 
> etc. Which are fine (& have been added in STLExtras) as free functions.

Okay, I'll just do that then.  Added as `index(Range, N)`, matching range-v3 
(though not the actual C++20 draft).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,17 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -58,11 +66,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
 }
+
+/// Optimized version for random iterators
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
+}
+
 }
 
 #endif
Index: llvm/include/llvm/ADT/STLExtras.h
===
--- llvm/include/llvm/ADT/STLExtras.h
+++ llvm/include/llvm/ADT/STLExtras.h
@@ -1573,6 +1573,14 @@
 }
 template  constexpr T *to_address(T *P) { return P; }
 
+template 
+auto index(R &&TheRange,
+  typename std::iterator_traits>::difference_type N)
+  -> decltype(TheRange.begin()[N]) {
+  assert(N < TheRange.end() - TheRange.begin() && "Index out of range!");
+  return TheRange.begin()[N];
+}
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STLEXTRAS_H
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = findInfo(Arg);
 
 if (Entry == P

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D67647#1674781 , @comex wrote:

> In D67647#1674773 , @dblaikie wrote:
>
> > I wasn't expecting it to involve all this work - but instead to change 
> > "arguments()" to return ArrayRef. Though perhaps you didn't go that route 
> > to allow future fixes to the strict aliasing (by having an adapting 
> > iterator - or perhaps arguments() already uses such an iterator that 
> > doesn't hit the strict aliasing issue?) needing to do even more cleaunp 
> > work?
>
>
> Yeah, `arguments()` returns an adapting iterator.


Right - I was suggesting that could be changed. Would it be OK to you to change 
arguments() to return ArrayRef? Or would you rather avoid that to avoid baking 
in more dependence on that alias violation?

>>> - Add an `operator[]` implementation to `iterator_range`.
>> 
>> I'm not sure that's appropriate - not all types with begin() and end() (even 
>> those with random access iterators) would have op[], so it doesn't seem 
>> appropriate to add it/depend on it?
> 
> Random access iterators are supposed to have `operator[]`, according to:
>  http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

Yep, I'm with you there

> But the use of a template ensures that it doesn't cause an error with types 
> that don't have `operator[]`, whether they are marked as random-access or not.
> 
> I checked the spec for "real" C++20 ranges... it seems that random access 
> ranges don't always have `operator[]`,

Yeah, that's where I've got different feelings - a general purpose utility to 
take any range (thing with begin/end) over random access iterators and do 
indexed lookup, I'd be fine with that (implemented as "r.begin()[n]") but 
adding op[] to the range itself then leads code to depend on that feature which 
isn't a general purpose range feature - so changing this iterator_range to some 
other range implementation might break that code - it'd be nice to keep 
iterator_range's interface minimal to allow implementation flexibility in the 
APIs that expose it.

Same reason I've pushed back on adding things like "empty()" or "size()", etc. 
Which are fine (& have been added in STLExtras) as free functions.

> but "view_interface" (which is a type of range) does:
>  https://eel.is/c++draft/view.interface

Fair - guess they've gone in a slightly different direction than we have in 
LLVM for now. I don't feel super strongly about it - but a bit.

> Anyway, I don't think it hurts to have it on this little imitation.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

In D67647#1674773 , @dblaikie wrote:

> I wasn't expecting it to involve all this work - but instead to change 
> "arguments()" to return ArrayRef. Though perhaps you didn't go that route to 
> allow future fixes to the strict aliasing (by having an adapting iterator - 
> or perhaps arguments() already uses such an iterator that doesn't hit the 
> strict aliasing issue?) needing to do even more cleaunp work?


Yeah, `arguments()` returns an adapting iterator.

>> - Add an `operator[]` implementation to `iterator_range`.
> 
> I'm not sure that's appropriate - not all types with begin() and end() (even 
> those with random access iterators) would have op[], so it doesn't seem 
> appropriate to add it/depend on it?

Random access iterators are supposed to have `operator[]`, according to:
http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

But the use of a template ensures that it doesn't cause an error with types 
that don't have `operator[]`, whether they are marked as random-access or not.

I checked the spec for "real" C++20 ranges... it seems that random access 
ranges don't always have `operator[]`, but "view_interface" (which is a type of 
range) does:
https://eel.is/c++draft/view.interface

Anyway, I don't think it hurts to have it on this little imitation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D67647#1674745 , @comex wrote:

> Here's a new version of the patch that uses iterator ranges instead of 
> `ArrayRef`, to avoid adding new uses of `CallExpr::getArgs()` in case it has 
> to be removed in the future due to the strict aliasing issue.


Not related to the strict aliasing issue - just seemed like it'd be tidier than 
reconstructing ranges manually from getArgs and getNumArgs.

> To support this, the following additional changes are included:

I wasn't expecting it to involve all this work - but instead to change 
"arguments()" to return ArrayRef. Though perhaps you didn't go that route to 
allow future fixes to the strict aliasing (by having an adapting iterator - or 
perhaps arguments() already uses such an iterator that doesn't hit the strict 
aliasing issue?) needing to do even more cleaunp work?

> 
> 
> - Fix `CastIterator`'s use of `iterator_adaptor_base` so that `operator[]` 
> isn't broken.
> - Add an `operator[]` implementation to `iterator_range`.

I'm not sure that's appropriate - not all types with begin() and end() (even 
those with random access iterators) would have op[], so it doesn't seem 
appropriate to add it/depend on it?

> - Improve the existing `llvm::drop_begin` helper to assert that it doesn't go 
> out of bounds.  (The implementation would be a lot less ugly if we could use 
> C++17 features; oh well.)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex updated this revision to Diff 220771.
comex added a comment.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.

Here's a new version of the patch that uses iterator ranges instead of 
`ArrayRef`, to avoid adding new uses of `CallExpr::getArgs()` in case it has to 
be removed in the future due to the strict aliasing issue.

To support this, the following additional changes are included:

- Fix `CastIterator`'s use of `iterator_adaptor_base` so that `operator[]` 
isn't broken.

- Add an `operator[]` implementation to `iterator_range`.

- Improve the existing `llvm::drop_begin` helper to assert that it doesn't go 
out of bounds.  (The implementation would be a lot less ugly if we could use 
C++17 features; oh well.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67647

Files:
  clang/include/clang/AST/Stmt.h
  clang/lib/Analysis/Consumed.cpp
  llvm/include/llvm/ADT/iterator_range.h

Index: llvm/include/llvm/ADT/iterator_range.h
===
--- llvm/include/llvm/ADT/iterator_range.h
+++ llvm/include/llvm/ADT/iterator_range.h
@@ -20,9 +20,17 @@
 
 #include 
 #include 
+#include 
 
 namespace llvm {
 
+template 
+constexpr bool is_random_iterator() {
+  return std::is_same<
+typename std::iterator_traits::iterator_category,
+std::random_access_iterator_tag>::value;
+}
+
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
@@ -44,6 +52,15 @@
 
   IteratorT begin() const { return begin_iterator; }
   IteratorT end() const { return end_iterator; }
+
+  template 
+  decltype(std::declval<_IteratorT>()[0]) operator[](
+  typename std::iterator_traits<_IteratorT>::difference_type N) const {
+static_assert(is_random_iterator(),
+  "Must be a random access iterator to use []");
+assert(N < end_iterator - begin_iterator && "Index out of range!");
+return begin_iterator[N];
+  }
 };
 
 /// Convenience function for iterating over sub-ranges.
@@ -58,11 +75,31 @@
   return iterator_range(std::move(p.first), std::move(p.second));
 }
 
+/// Non-random-iterator version
 template 
-iterator_range()))> drop_begin(T &&t,
-  int n) {
-  return make_range(std::next(adl_begin(t), n), adl_end(t));
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  for (int i = 0; i < n; i++) {
+assert(begin != end);
+++begin;
+  }
+  return make_range(begin, end);
 }
+
+/// Optimized version for random iterators
+template 
+auto drop_begin(T &&t, int n) ->
+  typename std::enable_if(),
+  iterator_range>::type {
+  auto begin = adl_begin(t);
+  auto end = adl_end(t);
+  assert(end - begin >= n && "Dropping more elements than exist!");
+  return make_range(std::next(begin, n), end);
+}
+
 }
 
 #endif
Index: clang/lib/Analysis/Consumed.cpp
===
--- clang/lib/Analysis/Consumed.cpp
+++ clang/lib/Analysis/Consumed.cpp
@@ -494,8 +494,10 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+
+  using ArgRange = llvm::iterator_range;
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArgRange args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +610,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArgRange Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = fi

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D67647#1673363 , @comex wrote:

> Ugh, it looks like `getArgs()` is a massive strict aliasing violation.  And 
> it's used in enough different places in Clang that fixing the violation may 
> require extensive refactoring... I've reported the issue as 
> https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix 
> it :)
>
> However, I can at least update this patch to avoid using getArgs().


Yeah, existing technical debt - I don't think your situation is making that any 
worse or better, so no worries about it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex added a comment.

Ugh, it looks like `getArgs()` is a massive strict aliasing violation.  And 
it's used in enough different places in Clang that fixing the violation may 
require extensive refactoring... I've reported the issue as 
https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it 
:)

However, I can at least update this patch to avoid using getArgs().


Repository:
  rC Clang

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Analysis/Consumed.cpp:752
+  handleCall(Call, nullptr,
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+ FunDecl);

probably use "Call->arguments()" here & in the other places that need an range 
of the arguments? (& if that can be an ArrayRef - updating CallExpr's 
arguments() and the arg_range and const_arg_range to use/be ArrayRef would be 
good)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67647



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

[PATCH] D67647: [Consumed] Refactor handleCall to take function argument list. NFC.

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added subscribers: cfe-commits, dmgreen, kristof.beyls.
Herald added a project: clang.

Currently, `handleCall` takes a `CallExpr` as an argument and retrieves the 
arguments from there.  This changes it to take the argument list as a separate 
argument of type `ArrayRef`.

The main motivation that I want `VisitCXXConstructExpr` to also be able to 
invoke `handleCall`, even though `CXXConstructExpr` is not a subclass of 
`CallExpr`.  But actually adding that will wait for a future patch.

However, this also fixes a minor bug in `VisitCXXOperatorCallExpr`:

  if (const auto *MCall = dyn_cast(Call))
handleCall(MCall, MCall->getImplicitObjectArgument(), FunDecl);
  else
handleCall(Call, Call->getArg(0), FunDecl);

`Call` here is a `CXXOperatorCallExpr`, and `CXXMemberCallExpr` is a sibling 
class, so the cast will never succeed.  CXXMemberCallExprs go through a 
separate visitor, `VisitCXXMemberCallExpr`.

On the other hand, this logic may be intended to reflect the fact that C++ 
operators can be declared as either methods or free functions.  The correct way 
to differentiate these is shown at the beginning of handleCall:

  unsigned Offset = 0;
  if (isa(Call) && isa(FunD))
Offset = 1;  // first argument is 'this'

For `CXXOperatorCallExpr`s, if the decl is a `CXXMethodDecl`, the first 
argument is `this`; otherwise there is no `this`.

Going back to the other first: currently, since the `dyn_cast` always fails, it 
always passes `Call->getArg(0)` as `ObjArg` (i.e. the expression representing 
`this`), even for operators declared as free functions.  However, this is 
harmless, because `ObjArg` is only used if the function is marked as one of 
`set_typestate` or `test_typestate`, or `callable_when`, yet those attributes 
are only allowed on methods.  `Call->getArg(0)` will crash if there are zero 
arguments, but the only kind of non-method operator function allowed to have 
zero arguments is a user-defined literal, and those do not produce 
`CXXOperatorCallExpr`s.

The bug could be fixed by changing the first snippet to check for 
`CXXMethodDecl` like the second one, but this refactor makes things cleaner by 
only having to check in one place.


Repository:
  rC Clang

https://reviews.llvm.org/D67647

Files:
  lib/Analysis/Consumed.cpp

Index: lib/Analysis/Consumed.cpp
===
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -494,8 +494,8 @@
   void checkCallability(const PropagationInfo &PInfo,
 const FunctionDecl *FunDecl,
 SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-  const FunctionDecl *FunD);
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+  ArrayRef Args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +608,21 @@
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArrayRef Args,
  const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa(Call) && isa(FunD))
-Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
 // Skip variable argument lists.
-if (Index - Offset >= FunD->getNumParams())
+if (Index >= FunD->getNumParams())
   break;
 
-const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+const ParmVarDecl *Param = FunD->getParamDecl(Index++);
 QualType ParamType = Param->getType();
 
-InfoEntry Entry = findInfo(Call->getArg(Index));
+InfoEntry Entry = findInfo(Arg);
 
 if (Entry == PropagationMap.end() || Entry->second.isTest())
   continue;
@@ -636,7 +635,7 @@
 
   if (ParamState != ExpectedState)
 Analyzer.WarningsHandler.warnParamTypestateMismatch(
-  Call->getArg(Index)->getExprLoc(),
+  Arg->getExprLoc(),
   stateToString(ExpectedState), stateToString(ParamState));
 }
 
@@ -749,7 +748,9 @@
 return;
   }
 
-  handleCall(Call, nullptr, FunDecl);
+  handleCall(Call, nullptr,
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+ FunDecl);
   propagateReturnType(Call, FunDecl);
 }
 
@@ -805,7 +806,8 @@
   if (!MD)
 return;
 
-  handleCall(Call, Call->getImplicitObjectArgument(), MD);
+  handleCall(Call, Call->getImplicitObjectA