Necessary Maintenance #24207
Outdated Wiki Instructions for creating indirection layer
0%
Description
This is a tiny detail.
In this link there are instructions to create an indirection layer to a vector of handles: https://cdcvs.fnal.gov/redmine/projects/larsoft/wiki/Using_art_in_LArSoft#artValidHandle-and-artHandle
It says:
std::vector<mp::MyProd const*> ptrColl(prodvec.size());
std::transform(prodvec.begin(), prodvec.end(), ptrColl.begin(), &std::addressof);
I couldn't get it to compile. Googling I found this explanation: https://stackoverflow.com/questions/45670453/gcc-7-compilation-error-when-using-stdaddressof
Inside, one of the comments to the accepted answer recommends a couple of solutions, I got it to work with the lambda function:
std::vector<mp::MyProd const*> ptrColl(prodvec.size());
std::transform(prodvec.begin(), prodvec.end(), ptrColl.begin(), [](auto& p) {return std::addressof(p);});
History
#1 Updated by Gianluca Petrillo 10 months ago
Thank you for reporting.
The line is getting complicate enough that it might be good to also suggest a precooked solution.
LArSift currently has a util::makePointerVector() (larcorealg:source:larcorealg/CoreUtils/SortByPointers.h), although if it is going to be advertised, maybe it should move into a more obvious header.
In alternative, do cetlib or art offer anything equivalent?
1 The added feature of that function is that it unwraps std::unique_ptr<T> to deliver a collection of T*.
#2 Updated by Kyle Knoepfel 10 months ago
- Status changed from New to Under Discussion
Gianluca, let's talk about this. We should certainly fix the documentation, but I think the guidance may change now that the C++ standard library is moving toward supporting views.
#3 Updated by Kyle Knoepfel 10 months ago
I actually think Iker's suggestion is no more complicated than the current documentation. I also think there is little use in recommending std::addressof as I suspect that for all actual types of interest, none of them overloads the reference operator. To that end, one could simply write:
#include "cetlib/container_algorithms.h"
std::vector<mp::MyProd const*> ptrColl(size(prodvec));
cet::transform_all(prodvec, begin(ptrColl), [](auto& p){ return &p; });
If the lambda expression must be named, I suppose LArSoft could name it lar::to_address, but I'm not sure it's worth it for how often this is needed and for the actual work being done by the closure object.
#4 Updated by Gianluca Petrillo 10 months ago
And I just noticed that util::takeAddress() in larcorealg:source:larcorealg/CoreUtils/operations.h (https://home.fnal.gov/~petrillo/icarus/code/larsoftobj/html/operations_8h.html) was specifically introduced to solve the reported problem.
#5 Updated by Kyle Knoepfel 9 months ago
- Status changed from Under Discussion to Feedback
Gianluca, please look at the updated wiki and let me know if it's acceptable.
#6 Updated by Gianluca Petrillo 9 months ago
It looks good to me.
I added a footnote explaining the unqualified begin() calls, since argument-dependent lookup is not an extensively taught feature.
Feel free to fix it if any misunderstanding is undermining it.
Edit: I have also added a mention to the relevant header.
#7 Updated by Kyle Knoepfel 9 months ago
- % Done changed from 0 to 100
- Status changed from Feedback to Closed
The footnote is helpful, Gianluca. Thanks for adding it, and thanks to Iker for reporting this issue.
#8 Updated by Gianluca Petrillo 9 months ago
- % Done changed from 100 to 0
- Status changed from Closed to Under Discussion
Actually the second example is not working, as the intended use of util::takeAddress is to return a functor rather than taking an address (see the online documentation).
Maybe it's worth changing that, e.g.constexpr auto takeAddress = [](auto& obj){ return std::addressof(obj); }; (being constexpr and therefore inline this definition can live in a header, right?).
Or we can leave the code as it is, and change the documentation (util::takeAddress() instead of util::takeAddress).
#9 Updated by Kyle Knoepfel 9 months ago
That's fine, Gianluca. Do you mind making the code change and creating a PR?
#10 Updated by Kyle Knoepfel 9 months ago
- Status changed from Under Discussion to Feedback
#11 Updated by Kyle Knoepfel 9 months ago
- Assignee set to Kyle Knoepfel
- Status changed from Feedback to Assigned