hard to say if it's more readable without seeing it - if you could attach a patch, if it's easy enough/you worked it up before, might be worth comparing/contrasting
On Tue, Jun 13, 2017 at 12:28 AM Dean Michael Berris via Phabricator < revi...@reviews.llvm.org> wrote: > 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 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
