Page MenuHomePhabricator

[ADT] Add non-const operator* to iterator_adaptor_base and df_iterator.
Changes PlannedPublic

Authored by fhahn on Nov 26 2019, 11:17 AM.

Details

Summary

The missing non-const operator* implementation are causing issues when
using filter_iterator with df_iterator: without the non-const
implementation, we can only return const references to the underlying
values.

This patch also adjusts the return type of operator* cont in
interator_adaptor_base.

Diff Detail

Event Timeline

fhahn created this revision.Nov 26 2019, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 11:17 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript

Build result: FAILURE -
Log files: console-log.txt, CMakeCache.txt

failed because connection timeout.

Could you add some test coverage (showing what would break, specifically - an iterator with only a const op* being adapted that would previously (well, previous to this patch, so currenctly in tree - might just take updating an existing iterator in the test case to fall into this situation) fail to compile) to llvm/unittests/ADT/IteratorTest.cpp

fhahn added a comment.Dec 2 2019, 12:00 PM

Could you add some test coverage (showing what would break, specifically - an iterator with only a const op* being adapted that would previously (well, previous to this patch, so currenctly in tree - might just take updating an existing iterator in the test case to fall into this situation) fail to compile) to llvm/unittests/ADT/IteratorTest.cpp

I just gave it a try with the iterators available in the unit tests, but so far I did not manage to replicate the compilation failure. My motivation is the vpbb_iterator_adaptor in D70734, which cannot be used to iterate over const VPlan objects. @dblaikie do you think this patch actually makes sense or just papers over a different issue?

Could you add some test coverage (showing what would break, specifically - an iterator with only a const op* being adapted that would previously (well, previous to this patch, so currenctly in tree - might just take updating an existing iterator in the test case to fall into this situation) fail to compile) to llvm/unittests/ADT/IteratorTest.cpp

I just gave it a try with the iterators available in the unit tests, but so far I did not manage to replicate the compilation failure. My motivation is the vpbb_iterator_adaptor in D70734, which cannot be used to iterate over const VPlan objects. @dblaikie do you think this patch actually makes sense or just papers over a different issue?

Ah, OK - well it sounds like there are two independent changes here, then. (which I guess was sort of obvious from the changes) - I'm guessing, currently, the depth first iterator would not work as the underlying iterator to the iterator_adaptor_base? So this patch is currently making two changes to "meet in the middle"? (ie: they're still incompatible until both these changes are made)

Does the issue not reproduce if you make an underlying iterator with only a const op* (that returns a const ref) & wrap that with iterator_adaptor_base?

fhahn planned changes to this revision.Dec 5 2019, 1:32 PM