Swift: extract lazy declarations #12335
Conversation
AlexDenisov
force-pushed
the
alexdenisov/extract-lazy-declarations
branch
3 times, most recently
from
0dde5ab to
abeb31f
Compare
AlexDenisov
force-pushed
the
alexdenisov/extract-lazy-declarations
branch
from
abeb31f to
ded0e57
Compare
AlexDenisov
changed the title
|
I'll start DCA as soon as the PR is green |
swift/extractor/SwiftExtractor.cpp
Outdated
| if (primaryFile) { | ||
| return resolvePath(primaryFile->getFilename()); | ||
| } | ||
| if (lazyDeclaration) { | ||
| static SwiftMangler mangler; |
There was a problem hiding this comment.
hmm, that's a singleton in disguise.
Not that I'm completely against using a singleton here, especially a (probably) stateless singleton like this, but then I would make it a more explicit singleton everywhere. That is, I'd make SwiftMangler's constructor private, give it a
static SwiftMangler& instance() {
static SwiftMangler ret;
return ret;
}and use that everywhere including the Decl translator.
By the way, how much stateless is this mangler? I don't think the original swift mangler is truly stateless, but we might be calling only state-invariant methods on it. If that's the case, maybe I'd make the mangledName method const and mark the internal swift mangler as mutable.
There was a problem hiding this comment.
I ended up switching to a simple local variable instead. It doesn't have to be a singleton, and there is actually no good reason for it to be.
There was a problem hiding this comment.
yeah, you are right. Maybe the only reason we could consider to turn it into a singleton, would be for mocking purposes in unit tests (that is, we could introduce a mangler singleton settable in tests to a mock). As we don't really do those, local variables and class members are good enough!
swift/extractor/SwiftExtractor.cpp
Outdated
| std::vector<const swift::Decl*> worklist; | ||
| std::copy(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations), | ||
| std::back_inserter(worklist)); |
There was a problem hiding this comment.
when you create a vector from scratch from another container, you can pass the iterators directly to the constructor
| std::vector<const swift::Decl*> worklist; | |
| std::copy(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations), | |
| std::back_inserter(worklist)); | |
| std::vector<const swift::Decl*> worklist(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations)); |
although, see my other comment on this block
swift/extractor/SwiftExtractor.cpp
Outdated
| std::vector<const swift::Decl*> worklist; | ||
| std::copy(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations), | ||
| std::back_inserter(worklist)); |
There was a problem hiding this comment.
| std::vector<const swift::Decl*> worklist; | |
| std::copy(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations), | |
| std::back_inserter(worklist)); | |
| std::vector<const swift::Decl*> worklist(std::begin(state.pendingDeclarations), std::end(state.pendingDeclarations)); |
| std::unordered_set<const swift::Decl*> emittedDeclarations; | ||
| std::unordered_set<const swift::Decl*> pendingDeclarations; |
There was a problem hiding this comment.
some short comments explaining these would be welcome I think
| @@ -12,6 +12,7 @@ enum class TrapType { | |||
| module, | |||
| invocation, | |||
| linkage, | |||
| lazy_declarations, | |||
There was a problem hiding this comment.
cosmetic nit for consistency with other consts:
| lazy_declarations, | |
| lazy_declaration, |
No description provided.