[cxx-interop] [NFC] Refactor ClangDerivedConformances #85932
Conversation
j-hui
requested review from
Xazax-hun,
beccadax,
egorzhdan,
ian-twilightcoder and
susmonteiro
as code owners
|
@swift-ci please test |
j-hui
force-pushed
the
unify-cxx-derived-conformances
branch
from
16ec563 to
5bf6f77
Compare
|
@swift-ci please test |
Xazax-hun
left a comment
Xazax-hun
left a comment
There was a problem hiding this comment.
I like this refactoring overall! Some minor comments inline.
| multiset, | ||
| pair, | ||
| map, | ||
| unordered_map, |
There was a problem hiding this comment.
I vaguely remember that there was a reason why unordered_mult(map|set) is not conformed to anything. @egorzhdan do you remember? I wonder if it would be useful to add some comment here about them so people don't wonder why they are missing from here.
There was a problem hiding this comment.
I don't remember off the top of my head, no. Supporting those types would be a nice future improvement!
| // Types for which the insert method is considered unsafe, | ||
| // due to potential iterator invalidation. | ||
| return llvm::StringSwitch<bool>(parentDecl->getName()) | ||
| .Cases({"set", "unordered_set", "multiset"}, true) |
There was a problem hiding this comment.
Do we want to cover unordered_multi(map|set) here?
There was a problem hiding this comment.
I think it would be better to do that in a separate PR to make reviewing easier, since this one aims to be an NFC.
| multiset, | ||
| pair, | ||
| map, | ||
| unordered_map, |
There was a problem hiding this comment.
I don't remember off the top of my head, no. Supporting those types would be a nice future improvement!
| // Types for which the insert method is considered unsafe, | ||
| // due to potential iterator invalidation. | ||
| return llvm::StringSwitch<bool>(parentDecl->getName()) | ||
| .Cases({"set", "unordered_set", "multiset"}, true) |
There was a problem hiding this comment.
I think it would be better to do that in a separate PR to make reviewing easier, since this one aims to be an NFC.
getClangOwningModule() no longer takes a parameter named `MI` nor does it return an optional (it just returns a poitner).
j-hui
force-pushed
the
unify-cxx-derived-conformances
branch
2 times, most recently
from
66fcc75 to
ad4467b
Compare
|
@swift-ci please test |
This refactoring restructures the code in a couple of ways: - move all the "conformToXXXIfNeeded()" functions into a single function and remove them from the ClangDerivedConformances.h interface - unify the check for isInStdNamespace() for the C++ stdlib types - unify the name comparisons into a single llvm::StringSwitch - unify redundant nullptr assert()s, and upgrade them to ASSERT() - move the known stdlib type name-checking logic out of the "conformToCxxSTDLIBTYPEIfNeeded()" functions, and rename them to drop the "IfNeeded" suffix. This patch also introduces a local CxxStdType enum class that enumerates (and thus documents) all of the known (and thus supported) types from the C++ stdlib types. The ultimate goal is to organize the code such that by the time we call "conformToCxxSTDLIBTYPE()" we are already sure that the type is (or at least claims to be, based on its name and DeclContext) the C++ stdlib type we think it is, rather than interleaving such checks with the conformance synthesis logic. The main observable difference should be the logging: now, we will no longer have PrettyStackTraceDecls logging for C++ stdlib types like "conforming to CxxVector" unless we are already certain we are looking at a relevant type, e.g., std::vector. Otherwise, the functional behavior of this code should be the same as before.
j-hui
force-pushed
the
unify-cxx-derived-conformances
branch
from
ad4467b to
c8ecb1f
Compare
|
@swift-ci please test |
3 participants
This refactoring restructures the code in a couple of ways:
and remove them from the ClangDerivedConformances.h interface
"conformToCxxSTDLIBTYPEIfNeeded()" functions, and rename them to drop
the "IfNeeded" suffix.
This patch also introduces a local CxxStdType enum class that enumerates
(and thus documents) all of the known (and thus supported) types from
the C++ stdlib types.
The ultimate goal is to organize the code such that by the time we call
"conformToCxxSTDLIBTYPE()" we are already sure that the type is (or at
least claims to be, based on its name and DeclContext) the C++ stdlib
type we think it is, rather than interleaving such checks with the
conformance synthesis logic.
The main observable difference should be the logging: now, we will no
longer have PrettyStackTraceDecls logging for C++ stdlib types like
"conforming to CxxVector" unless we are already certain we are looking
at a relevant type, e.g., std::vector.
Otherwise, the functional behavior of this code should be the same as
before.