There is no single ticket that is best to review this work descriptions are spread over many tickets but all the work is on just one branch. This ticket will serve well enough. Thank you for agreeing to do this review. The work is all on the tickets/DM-7170 branch on afw. For your part of the review, please review SpanSets.h, SpanSetsFunctorGetters.h and the corresponding parts of the unit test. Within SpanSets.h you are only responsible for reviewing the applyFunctor(Impl) functions. There are a few class methods which make use of that functionality, and you can decide for yourself if you want to look into those or not. If there are any questions, please send me a message here or on Slack.

Does the branch have a pull request? I can't review without one.

Feel free to open one, Bob Armstrong is doing the other part of the review. I don't know if you two want to share one pull request or have two. I think one will work fine, but I will let you two decide

Sorry for taking so long to get back to you. I've reviewed the code and left comments on GitHub. The biggest issue is documentation; the class needs to conform to DM's current documentation standards before it can be merged. I'd also like to see some improvements made to both the class code (in particular, I think the current names make the code harder to understand than it otherwise would be) and to unit tests. I don't see any issues with the variadic templates themselves, but I'd like to take another look once the code is clearer.

BTW, I just found out that if the reviewer opens the pull request, then GitHub won't let them either approve the PR or request revisions.

Krzysztof Findeisen John Swinbank I guess that this is due to the new Github review workflow. Perhaps we should make it policy that the person that creates the ticket also always creates the PR?

In fact, it already is policy — it just isn't widely followed. I'll post something on clo to remind people.

Krzysztof Findeisen I think I have addressed most of your concerns, though some points I changed to something in between your comments and what I had. When You get a chance, will you take a look at the updated code? There is a commit for code updates and one for unit test updates. These will of course be squashed down after review.

Looks much better, though I still have some concerns about const-correctness and unit test coverage. Feel free to merge after you've gone through the comments on GitHub.

merged to master