This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement C++ Range-for loops
ClosedPublic

Authored by tbaeder on Dec 31 2022, 7:23 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Dec 31 2022, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 31 2022, 7:23 AM
tbaeder requested review of this revision.Dec 31 2022, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 31 2022, 7:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Jan 18 2023, 10:32 AM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
404–409

Under what circumstances is there not a condition for a range-based for loop?

418–419

Under what circumstances is there not an increment?

clang/test/AST/Interp/loops.cpp
274

You should also add failure tests where the range-based for loop is not valid in a constant expression. Especially interesting cases would be ones involving lifetime problems, such as: https://godbolt.org/z/3EE7f8rdE

tbaeder updated this revision to Diff 491643.Jan 24 2023, 12:45 AM
tbaeder marked an inline comment as done.
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
418–419

The answer to both is that I looked at the code for regular for loops and assumed that the restrictions are the same.

But looks like there aren't and even isBody() always returns non-null.

tbaeder marked an inline comment as done.Jan 24 2023, 12:57 AM
tbaeder added inline comments.
clang/test/AST/Interp/loops.cpp
274

Oof, that reproducer doesn't work right now for several reasons :/ Can you come up with something simpler?

tbaeder updated this revision to Diff 491674.Jan 24 2023, 2:08 AM
aaron.ballman added inline comments.Jan 31 2023, 12:03 PM
clang/test/AST/Interp/loops.cpp
274

Not trivially, no. Also, that case is now fixed by https://wg21.link/p2644r0 for C++23 due to lifetime extension that we probably don't model yet.

aaron.ballman accepted this revision.Feb 28 2023, 6:09 AM

LGTM; I suspect there's still lifetime issues we are not catching here, but this is incrementally forward progress and we can test the lifetime issues once we have more of the interpreter implemented.

This revision is now accepted and ready to land.Feb 28 2023, 6:09 AM
This revision was landed with ongoing or failed builds.Mar 2 2023, 12:01 AM
This revision was automatically updated to reflect the committed changes.