[PATCH] D56122: [CallSite removal] Migrate the statepoint GC infrastructure to use the `CallBase` class rather than `CallSite` wrappers.

Sun Feb 10 23:41:11 PST 2019

chandlerc marked an inline comment as done.
chandlerc added a comment.
Herald added a project: LLVM.

In D56122#1360749 <https://reviews.llvm.org/D56122#1360749>, @reames wrote:

> LGTM w/one requested change before submit.  If you strongly disagree, feel free to land and discuss afterwards.  I don't want this blocked on me finding time to re-review.

Thanks, made the change and landing.

> Minor (can be easily cleaned up later): Using Call seems a bit confusing as a variable name.  I'd have preferred CB for consistency with other naming.

I prefer words over initialisms where there are reasonable words to use. That said, if you want to use `CB` here, :: shrug ::...

> I see a bunch of follow on simplifications, but I can do a wave of small commits once this lands and bakes for a bit.

Yeah, happy for you to do subsequent simplifications.

================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2379

-// Handles both return values and arguments for Functions and CallSites.
+// Handles both return values and arguments for Functions and calls.
 template <typename AttrHolder>
----------------
reames wrote:
> This looks like a spurious unrelated change.  I also don't see why passing by pointer is better than reference here.  Can you remove this from the diff please?
Yeah, no idea how this got into the diff in the first place. Removed.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56122