This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Migrate DepthFirstIterator to use NodeRef
ClosedPublic

Authored by timshen on Aug 3 2016, 5:58 PM.

Details

Summary

Notice that the data layout is changed: instead of using
std::pair<PointerIntPair<NodeType*, 1>, ChildItTy>, now use
std::pair<NodeRef, Optional<ChildItTy>>.

Diff Detail

Event Timeline

timshen updated this revision to Diff 66744.Aug 3 2016, 5:58 PM
timshen retitled this revision from to [ADT] Migrate DepthFirstIterator to use NodeRef.
timshen updated this object.
timshen added reviewers: dblaikie, chandlerc.
timshen added a subscriber: llvm-commits.
timshen updated this revision to Diff 66746.Aug 3 2016, 6:02 PM

Updated comments to reflect the change.

dblaikie edited edge metadata.Aug 4 2016, 11:24 AM

Removing the ability to compare non-end iterators seems like a loss of functionality & would be pretty tricky for the average user (would catch them by surprise that only end iterators are comparable).

Unless there's a reason the design already precludes comparing non-end iterators, I'd say it's best to preserve the existing behavior there.

Other than that, it looks pretty reasonable.

include/llvm/ADT/DepthFirstIterator.h
111

I'd just write this as "if (!Opt)" - but up to you.

114–117

For a separate patch: any idea why this isn't a normal for (or even range-for) loop?

118

We aren't really using braced init like that much in the project so far. I'd probably stick with () when calling a ctor. (same feedback elsewhere)

timshen updated this revision to Diff 66875.Aug 4 2016, 4:22 PM
timshen edited edge metadata.

Updated as commented.

timshen marked 3 inline comments as done.Aug 4 2016, 4:25 PM
timshen added inline comments.
include/llvm/ADT/DepthFirstIterator.h
114–117

Oops I already fixed it. I don't think that it's a functional change, but just a modernization.

dblaikie accepted this revision.Aug 5 2016, 2:25 PM
dblaikie edited edge metadata.

Looks good, thanks! (I realize we've got to wait to finalize the Optional stuff first, of course)

This revision is now accepted and ready to land.Aug 5 2016, 2:25 PM
This revision was automatically updated to reflect the committed changes.
timshen marked an inline comment as done.
timshen updated this object.Aug 11 2016, 4:34 PM
timshen edited edge metadata.