[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

Dean Michael Berris via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 13 00:28:12 PDT 2017


dberris added a comment.

In https://reviews.llvm.org/D34052#778670, @dblaikie wrote:

> I take it there's already handling for these attributes on non-member functions?


Yes, we're just extending it to also apply to the case where it doesn't support this one case where we actually do need the implicit this argument

> There's probably a reason this code can't be wherever that code is & subtract one from the index? (reducing code duplication by having all the error handling, etc, in one place/once)

I tried doing it for the `checkFunctionOrMethodNumParams` function, but it ended up being much more complicated. :(

The choice is between adding a bool argument (e.g. AllowImplicitThis) in `checkFunctionOrMethodParameterIndex(...)` that's default to always true (to preserve existing behaviour) but the checks and implementation of that template has a number of assumptions as to whether the index is valid for member functions.

I can change this so that the logic is contained in `checkFunctionOrMethodParameterIndex(...)` if that's more readable?


https://reviews.llvm.org/D34052





More information about the cfe-commits mailing list