Coverage: fix handling of constructors and top-level decls (SR-7446) #15966
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need to instrument constructor initializers, instead of the delegating constructors which just call them. rdar://39460313
vedantk
changed the title
|
This scheme of using a designated constructor for profiling purposes is a bit brittle. One concrete issue with this is that swift ends up attempting to create distinct SILProfilers for different constructors of a nominal type, and stored property initializers we want coverage for may not be emitted in the designated constructor. A simpler idea is to store a map from nominal types to SILProfilers, and to then create a single merged profiler instance for all constructors of a nominal type. |
vedantk
force-pushed
the
cov-delegate-init
branch
from
9144c55
to
09f9207
Compare
vedantk
changed the title
|
@swift-ci test and merge |
1 similar comment
|
@swift-ci test and merge |
…zers This fixes coverage reporting for member initializers and cuts down on repeated AST traversals of pattern bindings within nominal type decls. This allows us to remove some defensive heuristic code which dealt with closures and if-exprs within member initializers.
vedantk
force-pushed
the
cov-delegate-init
branch
from
09f9207
to
9fa0c85
Compare
|
@swift-ci smoke test and merge |
1 similar comment
|
@swift-ci smoke test and merge |
vedantk
added a commit
to vedantk/swift
that referenced
this pull request
Apr 17, 2018
…pple#15966) * [Coverage] Instrument constructor initializers (SR-7446) We need to instrument constructor initializers, instead of the delegating constructors which just call them. rdar://39460313 * [Coverage] Remove dead code, NFC * [Coverage] Use a shared profiler for constructors and member initializers This fixes coverage reporting for member initializers and cuts down on repeated AST traversals of pattern bindings within nominal type decls. This allows us to remove some defensive heuristic code which dealt with closures and if-exprs within member initializers. (cherry picked from commit 8a003a4)
|
@vedantk What if a type has multiple designated initializers? |
|
Also unrelated, this caused a failure in the ASAN bot: |
|
@slavapestov If a type has multiple designated initializers, coverage for each should be reported separately (each constructor decl is visited). I'm taking a look at the asan failure now. |
|
Here's a PR to address the asan issue: #16012 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instrument constructor initializers, instead of the delegating constructors which just call them.
Once that's done, make sure that a parent region is available inside of member initializers and top-level code decls. This fixes a reporting bug for if-exprs by getting rid of the RegionStack.empty() case.
Remove some defensive heuristic code which dealt with closures and if-exprs within member initializers.