Skip to content

[cxx-interop] Implement operator[] for value return types (SR-14351). #36688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 9, 2021
Merged

[cxx-interop] Implement operator[] for value return types (SR-14351). #36688

merged 1 commit into from
Apr 9, 2021

Conversation

plotfi
Copy link
Contributor

@plotfi plotfi commented Mar 31, 2021

This builds on top of the work of Egor Zhdan's PR. It implements
T operator[] and does so largely by taking a path very much like the
const T &operator[] path.

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Mar 31, 2021
@plotfi
Copy link
Contributor Author

plotfi commented Apr 1, 2021

@zoecarver @egorzhdan Still working on cleaning up the tests. So far I kinda copied Egor's tests for the reference version. Stay tuned.

@gottesmm
Copy link
Contributor

gottesmm commented Apr 1, 2021

I haven't reviewed this, but just kicking off tests.

@gottesmm
Copy link
Contributor

gottesmm commented Apr 1, 2021

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 1, 2021

Build failed
Swift Test Linux Platform
Git Sha - ab1938a

@plotfi plotfi force-pushed the cxx-operator-subscript branch from ab1938a to 1c1c6ff Compare April 1, 2021 05:04
zoecarver
zoecarver reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good to me, thanks!

@zoecarver
Copy link
Contributor

@swift-ci please smoke test.

@plotfi plotfi force-pushed the cxx-operator-subscript branch 2 times, most recently from e94e5f8 to cb464d5 Compare April 1, 2021 07:07
zoecarver
zoecarver reviewed Apr 1, 2021
View reviewed changes
egorzhdan
egorzhdan reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks great! except for the case when operator[] returns a pointer.

@plotfi plotfi force-pushed the cxx-operator-subscript branch from cb464d5 to 4947e34 Compare April 2, 2021 20:51
zoecarver
zoecarver reviewed Apr 5, 2021
View reviewed changes
@plotfi plotfi force-pushed the cxx-operator-subscript branch from 4947e34 to b4dda83 Compare April 5, 2021 20:57
zoecarver
zoecarver reviewed Apr 5, 2021
View reviewed changes
@plotfi plotfi force-pushed the cxx-operator-subscript branch from b4dda83 to c60e1d0 Compare April 5, 2021 21:17
zoecarver
zoecarver reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic all looks good to me but I have a few more comments, sorry ;)

@plotfi plotfi force-pushed the cxx-operator-subscript branch 3 times, most recently from 3f7a871 to 17b1a05 Compare April 6, 2021 17:50
zoecarver
zoecarver reviewed Apr 6, 2021
View reviewed changes
@plotfi plotfi force-pushed the cxx-operator-subscript branch 2 times, most recently from c1e7f82 to d20b25d Compare April 7, 2021 02:07
@egorzhdan
Copy link
Contributor

@swift-ci please smoke test

egorzhdan
egorzhdan approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!
We'll also need to run the Windows CI before merging (I can't trigger it myself).

zoecarver
zoecarver approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one final comment. Other than that, it looks good to me! Are you able to commit yourself?

@zoecarver
Copy link
Contributor

@swift-ci please test Windows.

@plotfi plotfi force-pushed the cxx-operator-subscript branch from d20b25d to 67492b9 Compare April 7, 2021 17:50
@plotfi
Copy link
Contributor Author

plotfi commented Apr 7, 2021

Thanks @egorzhdan @zoecarver !

@zoecarver
Copy link
Contributor

@swift-ci please smoke test.

@plotfi
[cxx-interop] Implement operator[] for value return types (SR-14351).
864b3a4 
This builds on top of the work of Egor Zhdan. It implements
`T operator[]` and does so largely by taking a path very much like the
`const T &operator[]` path.
@plotfi plotfi force-pushed the cxx-operator-subscript branch from 67492b9 to 864b3a4 Compare April 8, 2021 04:18
@plotfi
Copy link
Contributor Author

plotfi commented Apr 8, 2021

Can someone kick off the Windows CI for me again? I just rewrote the name mangling in the silgen test based on some runs from a Windows build.

@compnerd
Copy link
Member

compnerd commented Apr 8, 2021

@swift-ci please test Windows platform

@plotfi
Copy link
Contributor Author

plotfi commented Apr 8, 2021

@zoecarver @compnerd Is the CI Busted? I don't see any results for mac/Linux and Windows has a bunch of failures building llvm with

C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1424~1.283\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\CodeGen -IS:\jenkins\workspace\swift-PR-windows\llvm-project\llvm\lib\CodeGen -Iinclude -IS:\jenkins\workspace\swift-PR-windows\llvm-project\llvm\include /GS- /Oy /Zc:inline /Zc:__cplusplus /Zi /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2    /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Folib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocBase.cpp.obj /Fdlib\CodeGen\CMakeFiles\LLVMCodeGen.dir\LLVMCodeGen.pdb /FS -c S:\jenkins\workspace\swift-PR-windows\llvm-project\llvm\lib\CodeGen\RegAllocBase.cpp
S:\jenkins\workspace\swift-PR-windows\llvm-project\llvm\lib\CodeGen\RegAllocBase.cpp(175): fatal error C1069: cannot read compiler command line

Can we rerun?

@drodriguez
Copy link
Contributor

@swift-ci please test Windows platform

@zoecarver
Copy link
Contributor

@swift-ci please smoke test macOS.

@zoecarver
Copy link
Contributor

@swift-ci please smoke test Linux.

@compnerd compnerd merged commit 68ff55a into swiftlang:main Apr 9, 2021
@varungandhi-apple
Copy link
Contributor

The test member-inline-silgen.swift is failing: https://ci.swift.org/job/swift-PR-macos-smoke-test/28749/consoleText

@varungandhi-apple varungandhi-apple mentioned this pull request Apr 9, 2021
Merged
@varungandhi-apple
Copy link
Contributor

Fix in #36833

@swift-ci swift-ci mentioned this pull request Apr 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@egorzhdan egorzhdan egorzhdan approved these changes

@zoecarver zoecarver zoecarver approved these changes

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@plotfi @gottesmm @swift-ci @zoecarver @egorzhdan @compnerd @drodriguez @varungandhi-apple