Project

General

Profile

Necessary Maintenance #24207

Outdated Wiki Instructions for creating indirection layer

Added by Iker de Icaza Astiz 7 months ago. Updated 6 months ago.

Status:
Assigned
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
03/20/2020
Due date:
% Done:

0%

Estimated time:
Experiment:
-
Duration:

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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 months ago

That's fine, Gianluca. Do you mind making the code change and creating a PR?

#10 Updated by Kyle Knoepfel 7 months ago

  • Status changed from Under Discussion to Feedback

#11 Updated by Kyle Knoepfel 6 months ago

  • Assignee set to Kyle Knoepfel
  • Status changed from Feedback to Assigned


Also available in: Atom PDF