[C++-Interop] Teach omitNeedlessWords to handle raw integer C-enums in C++ #42412
Conversation
|
@swift-ci please smoke test |
|
@swift-ci please smoke test. |
|
@zoecarver does the way I am going about this seem reasonable? I worry there could be typedef names that we don’t want to necessarily always rename here. |
|
Generally it seems reasonable, but we will probably need to make the check more specific. We probably want to see if this typedef has been replaced with an enum. Not exactly sure what the best way to do that is... maybe with a lookup, or a "global" map, or maybe using |
| @@ -2431,6 +2431,16 @@ DefaultArgumentKind ClangImporter::Implementation::inferDefaultArgument( | |||
| return DefaultArgumentKind::EmptyArray; | |||
| } | |||
| } | |||
| } else if (const clang::TypedefType *typedefType = | |||
| type->getAs<clang::TypedefType>()) { | |||
There was a problem hiding this comment.
This is maybe another reason that we should try harder to link typedef -> enum with some clang attr (this will be hard without naming the enum 😕)
There was a problem hiding this comment.
This is maybe another reason that we should try harder to link typedef -> enum with some clang attr (this will be hard without naming the enum 😕)
Yeah I was thinking something similar. Maybe we need another attr or something to link things. It would be potentially problematic though because it would assume folks are putting additional attrs on their enums (unless they just use CF_OPTIONS ).
Yeah that’s exactly my thought. I was thinking either by using some global map or by just checking to make sure what’s being typedefed is something like a built in or an NSInteger (the most naive way imo). Ok sounds like we are on the same page. |
|
Surprised that this isn't breaking anything. I'd expect something like |
|
@swift-ci Please Test Source Compatibility |
Yeah, I was also surprised this didnt break things for me locally. |
|
For the record: the source compatibility test succeeded. |
plotfi
force-pushed
the
c-enums-withOptions-omit
branch
from
3981849 to
dfec3ec
Compare
lib/ClangImporter/ImportType.cpp
Outdated
| // Get the AvailabilityAttr that would be set from CF/NS_OPTIONS | ||
| const clang::AvailabilityAttr *availabilityAttr = | ||
| typedefType->getDecl()->getAttr<clang::AvailabilityAttr>(); | ||
|
|
||
| if (availabilityAttr && availabilityAttr->getUnavailable()) { |
There was a problem hiding this comment.
Please use isUnavailableInSwift instead.
There was a problem hiding this comment.
Done. Should I leave enableObjCInterop to true?
plotfi
force-pushed
the
c-enums-withOptions-omit
branch
from
dfec3ec to
7307b38
Compare
…n C++ As I understand it, enums in C++ do not allow for bitwise operations that can also be assigned or returned as the same enum type. As a result there are macros in (Core)Foundation like NS/CF_OPTIONS that produce enums differently depending on #if __cplusplus or not. Because of this, code in omitNeedlessWordsInFunctionName and subsequently inferDefaultArgument in the ClangImporter that is in charge of replacing "needless words" from method and enum names does not trigger in the case of C++ because it is looking for EnumDecls that do not exists because they are actually typedefs on wrap integers or NSIntegers. This change attempts to do the renaming off of the typedef alone when such code exists. Special thanks to @bulbazord (Alex Langford) for taking the time to investigate this issue.
plotfi
force-pushed
the
c-enums-withOptions-omit
branch
from
7307b38 to
a0985a0
Compare
|
@swift-ci please smoke test. |
| } else if (const clang::TypedefType *typedefType = | ||
| type->getAs<clang::TypedefType>()) { | ||
| // Get the AvailabilityAttr that would be set from CF/NS_OPTIONS | ||
| if (importer::isUnavailableInSwift(typedefType->getDecl(), nullptr, true)) { |
There was a problem hiding this comment.
You can actually use if (isUnavailableInSwift(typdefType->getDecl())) here because you're in the Implementation (in my patch I had to use this because of layering).
There was a problem hiding this comment.
I see this is a static method, so never mind.
As I understand it, enums in C++ do not allow for bitwise operations
that can also be assigned or returned as the same enum type. As a result
there are macros in (Core)Foundation like NS/CF_OPTIONS that produce
enums differently depending on #if __cplusplus or not.
Because of this, code in omitNeedlessWordsInFunctionName and
subsequently inferDefaultArgument in the ClangImporter that is in charge
of replacing "needless words" from method and enum names does not
trigger in the case of C++ because it is looking for EnumDecls that dont
exists because they are actually typedefs on wrap integers or
NSIntegers.
This change attempts to do the renaming off of the typedef alone when
such code exists. However at the moment it is somewhat of a work in
progress.
Special thanks to @bulbazord (Alex Langford) for taking the time to investigate this issue.