[PATCH] D151198: [llvm][ADT] Fix invalid `reference` type of depth-first, breadth-first and post order iterators

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar accepted this revision.
kuhar added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/ADT/BreadthFirstIterator.h:53
   using pointer = value_type *;
-  using reference = value_type &;
+  using reference = const value_type &;
 
----------------
zero9178 wrote:
> kuhar wrote:
> > Could we add const conditionally, either with some type traits or define `reference` in terms of `decltype(*declval<pointer>())`?
> > 
> > Do you know if it there's any existing use of non-const references with these iterators?
> The result of `operator*` has always been a const-reference so there couldn't have been (or at the very least can't currently be) any real properly compiling code that relies on `reference` being non-const while `operator*` isn't. 
> 
>  Defining it to `decltype(*declval<pointer>())` would just be equal to `value_type&` no? That would at the very least not be possible in a non-const `operator*` and I think is intentionally not the case, since a `NodeRef&` would allow breaking the invariants of the iterators (would modify the `NodeRef` within `VisitQueue`)
> 
> Hope I understood your comment correctly.
Oh I see what you mean: the runtime usage used to depend on the definition of `operator*() const` and what this PR does is aligning the definition of the `reference` typedef to that usage.

I'd expect to see this operator return `const_reference` perhaps, but since there's no non-const version then this change seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151198