[PATCH] D79740: Align mapped_iterator::reference type with mapped_iterator::operator*() return value.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

I think it might be more obvious if it's implemented as two specializations, rather than one generic/one specialized - since they're kind of "equal" here (so it's not obvious in the current code that the primary template actually only applies when IsReference is true) - maybe add a comment before the boolean literal in the specializations to mention what it represents (so you don't have to go check the primary template declaration to see what the boolean is about)?

Should we call "F" every time the iterator is dereferenced? Or should it be done on increment... guess it can't be done on increment because you don't know at that point whether the new iterator is valid for dereferencing. If "V" is an Optional, then you could set it to None on increment, initialize it in op* if and only if it's None. That way it's only called once for the value, which might be important? Any idea what Boost or other iterators do here - there might be some useful prior art to learn from/model on?

(some testing would be good for this - unit tests)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79740/new/

https://reviews.llvm.org/D79740