[cxx-interop] Update CxxMethodBridging with snake_case #41617

cabmeurer · 2022-03-02T15:00:21Z

Update CxxMethodBridging to transform snake_case names

related: #40842

cabmeurer · 2022-03-02T15:04:28Z

Updated the CxxMethodBridging, let me know what changes/fixes or if there is anything else I should add to this pr.

zoecarver · 2022-03-02T17:14:19Z

Hey @cabmeurer. Really, sorry. I should have elaborated more in the forum post.

The idea behind this whole set of features is that unlike C++, Swift has a formalized style. As we import APIs we can try to be smart about this and transform them to adopt Swift's style (to a certain extent). It is unlikely that we'll ever want to transform something to snake_case in Swift.

Right now we will transform GetFoo -> foo and getUTF8String -> utf8String. However, we don't know how to transform snake_case, so get_foo turns into _foo and get_utf8_string -> _utf8_string which is not ideal. To fix this, you can update importNameAsCamelCaseName to handle the case where if (classifyNameKind() == NameKind::snake).

You can also look at that PR to see how to write the tests. Start by adding a snake_case struct to implicit-computed-properties.h and then seeing how this changes the output that is checked in implicit-computed-properties-module-interface.swift. Let me know if you need help with any of this.

That being said, this PR looks really good! If we wanted to import something as snake case, this patch would have been perfect :)

cabmeurer · 2022-03-02T17:29:06Z

Hey @cabmeurer. Really, sorry. I should have elaborated more in the forum post.

The idea behind this whole set of features is that unlike C++, Swift has a formalized style. As we import APIs we can try to be smart about this and transform them to adopt Swift's style (to a certain extent). It is unlikely that we'll ever want to transform something to snake_case in Swift.

Right now we will transform GetFoo -> foo and getUTF8String -> utf8String. However, we don't know how to transform snake_case, so get_foo turns into _foo and get_utf8_string -> _utf8_string which is not ideal. To fix this, you can update importNameAsCamelCaseName to handle the case where if (classifyNameKind() == NameKind::snake).

You can also look at that PR to see how to write the tests. Start by adding a snake_case struct to implicit-computed-properties.h and then seeing how this changes the output that is checked in implicit-computed-properties-module-interface.swift. Let me know if you need help with any of this.

That being said, this PR looks really good! If we wanted to import something as snake case, this patch would have been perfect :)

Ahh yeah that makes a lot more sense, will change this PR to allow for get_foo -> _foo and get_utf8_string ->_utf8_string

Thank you!

zoecarver · 2022-03-02T17:33:18Z

Ahh yeah that makes a lot more sense, will change this PR to allow for get_foo -> _foo and get_utf8_string ->_utf8_string

Well, that's what happens today. What we want is get_foo -> foo and get_utf8_string -> utf8String. Make sense?

cabmeurer · 2022-03-02T17:34:34Z

Oh yeah makes sense

edit: Okay, pushed that changes hopefully this is what we are wanting lol

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

zoecarver · 2022-03-02T19:18:32Z

Okay, pushed that changes hopefully this is what we are wanting lol

Yes! This is perfect. Nicely done!

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

zoecarver · 2022-03-03T00:15:58Z

This looks good but there are a couple issues with the tests. Are you able to run the tests locally?

cabmeurer · 2022-03-03T00:25:34Z

I ran this: utils/run-test --build-dir ../build/Xcode-DebugAssert/swift-macosx-x86_64 test/Interop/Cxx/

what is the best way to run tests locally?

so i am running utils/lit/lit.py -sv /swift-macosx-x86_64/test-macosx-x86_64/ now

cabmeurer · 2022-03-03T16:17:47Z

This looks good but there are a couple issues with the tests. Are you able to run the tests locally?

@zoecarver so i made some changes to the tests, that i think should fix it

thank you for being so patient

zoecarver · 2022-03-03T22:51:59Z

so i am running utils/lit/lit.py -sv /swift-macosx-x86_64/test-macosx-x86_64/ now

Yeah, that looks correct. And the tests are passing?

BTW if you just want to run the interop tests, you can run utils/lit/lit.py -sv /swift-macosx-x86_64/test-macosx-x86_64/Interop/Cxx.

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

cabmeurer · 2022-03-03T23:14:35Z

okay, so when i ran utils/lit/lit.py -sv /swift-macosx-x86_64/test-macosx-x86_64/Interop/Cxx
the output i got was:

Testing Time: 56.03s Unsupported: 8 Failed : 222 28 warning(s) in tests

zoecarver · 2022-03-03T23:31:32Z

Testing Time: 56.03s Unsupported: 8 Failed : 222 28 warning(s) in tests

Awesome! So the good news is the tests are running. The bad news is the tests are failing 😁

Why might they be failing... Can you post an output? Given that they're all failing, I suspect the stdlib isn't built. But not 100% sure.

stevapple · 2022-03-04T01:13:12Z

What we want is get_foo -> foo and get_utf8_string -> utf8String. Make sense?

Should we maintain a list of known abbreviations that should keep the same case? It’s not so good to see get_processed_utf8_string to be processedUtf8String in Swift, or get_processed_html_string to be processedHtmlString.

cabmeurer · 2022-03-04T01:54:38Z

What we want is get_foo -> foo and get_utf8_string -> utf8String. Make sense?

Should we maintain a list of known abbreviations that should keep the same case? It’s not so good to see get_processed_utf8_string to be processedUtf8String in Swift, or get_processed_html_string to be processedHtmlString.

I am into that idea, I can bring it up in the interop workgroup

cabmeurer · 2022-03-08T05:27:19Z

Awesome! So the good news is the tests are running. The bad news is the tests are failing 😁
Why might they be failing... Can you post an output? Given that they're all failing, I suspect the stdlib isn't built. But not 100% sure.

Hey @zoecarver, my tests are now all passing locally 🙌 let me know if there are any other changes I need to make

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift

zoecarver · 2022-03-08T16:21:24Z

Hey @zoecarver, my tests are now all passing locally 🙌 let me know if there are any other changes I need to make

Awesome! I'm glad you figured it out! Sorry I was sort of quiet over the past couple days.

cabmeurer · 2022-03-09T01:18:03Z

Hey @zoecarver, my tests are now all passing locally 🙌 let me know if there are any other changes I need to make

Awesome! I'm glad you figured it out! Sorry I was sort of quiet over the past couple days.

All good! Gave me some time to learn more about how our testing works and experiment

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift

bro4all · 2022-03-10T17:47:37Z

@swift-ci please test

cabmeurer · 2022-03-16T06:33:38Z

Hey @bro4all, thanks for looking over this! I made some changes, let me know what other changes i need to make

0x41c · 2022-03-17T16:54:55Z

include/swift/ClangImporter/CXXMethodBridging.h

@@ -96,6 +127,24 @@ struct CXXMethodBridging {
    if (output.empty())
      return output;

+    if (classifyNameKind() == NameKind::snake) {
+      for (std::size_t i = 0; i < output.size(); i++) {
+        size_t next = i + 1;


If the last character is an underscore, output[next] isn't checked to exist and is erased. We should check the validity of next.

Suggested change

size_t next = i + 1;

size_t next = i + 1;

if (output[next] == output.end())

return output;

cabmeurer · 2022-03-19T00:26:16Z

Closing this pull request and created a new one: Add snake_case to CXXMethodBridging
Finally had some time to fix my tests and make the necessary changes.

cabmeurer changed the title ~~[cxx-interop] Update CxxMethodBridging with SnakeCase~~ [cxx-interop] Update CxxMethodBridging with snake_case Mar 2, 2022

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch 3 times, most recently from fd92c9c to 1075dbc Compare March 2, 2022 18:41

zoecarver reviewed Mar 2, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Outdated Show resolved Hide resolved

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from 1075dbc to 040ac76 Compare March 2, 2022 20:09

cabmeurer requested a review from zoecarver March 2, 2022 20:09

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from 040ac76 to ae4cc13 Compare March 2, 2022 20:14

zoecarver reviewed Mar 3, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Outdated Show resolved Hide resolved

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from ae4cc13 to 211fd5e Compare March 3, 2022 00:23

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch 2 times, most recently from 275c08f to 3855614 Compare March 3, 2022 16:17

cabmeurer requested a review from zoecarver March 3, 2022 21:57

zoecarver reviewed Mar 3, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Outdated Show resolved Hide resolved

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from 3855614 to d4de548 Compare March 6, 2022 07:34

cabmeurer requested a review from zoecarver March 6, 2022 07:36

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from d4de548 to 064d457 Compare March 6, 2022 15:06

zoecarver added the c++ interop Feature: Interoperability with C++ label Mar 8, 2022

zoecarver reviewed Mar 8, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Show resolved Hide resolved

zoecarver reviewed Mar 8, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift Outdated Show resolved Hide resolved

bro4all reviewed Mar 10, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift Outdated Show resolved Hide resolved

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from 064d457 to 9bbb2fd Compare March 16, 2022 06:31

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from 9bbb2fd to df47aa4 Compare March 16, 2022 14:25

cabmeurer requested review from zoecarver and bro4all March 17, 2022 01:54

0x41c reviewed Mar 17, 2022

View reviewed changes

[cxx-interop] Update CxxMethodBridging with snake_case

436847b

cabmeurer force-pushed the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch from df47aa4 to 436847b Compare March 18, 2022 21:33

cabmeurer closed this Mar 19, 2022

cabmeurer deleted the cxx-interop/Add-snake_case-support-to-CXXMethodBridging branch March 19, 2022 00:26

[cxx-interop] Update CxxMethodBridging with snake_case #41617

[cxx-interop] Update CxxMethodBridging with snake_case #41617

Uh oh!

Conversation

cabmeurer commented Mar 2, 2022

Uh oh!

cabmeurer commented Mar 2, 2022

Uh oh!

zoecarver commented Mar 2, 2022

Uh oh!

cabmeurer commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoecarver commented Mar 2, 2022

Uh oh!

cabmeurer commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Mar 2, 2022

Uh oh!

Uh oh!

zoecarver commented Mar 3, 2022

Uh oh!

cabmeurer commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cabmeurer commented Mar 3, 2022

Uh oh!

zoecarver commented Mar 3, 2022

Uh oh!

Uh oh!

cabmeurer commented Mar 3, 2022

Uh oh!

zoecarver commented Mar 3, 2022

Uh oh!

stevapple commented Mar 4, 2022

Uh oh!

cabmeurer commented Mar 4, 2022

Uh oh!

cabmeurer commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Mar 8, 2022

Uh oh!

cabmeurer commented Mar 9, 2022

Uh oh!

Uh oh!

bro4all commented Mar 10, 2022

Uh oh!

cabmeurer commented Mar 16, 2022

Uh oh!

0x41c Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

cabmeurer commented Mar 19, 2022

Uh oh!

Uh oh!

cabmeurer commented Mar 2, 2022 •

edited

Loading

cabmeurer commented Mar 2, 2022 •

edited

Loading

cabmeurer commented Mar 3, 2022 •

edited

Loading

cabmeurer commented Mar 8, 2022 •

edited

Loading