TypeGraph: Create dummy containers #185
Conversation
ajor
force-pushed
the
allocator-pass-through
branch
3 times, most recently
from
8fc35cb to
ea1c442
Compare
|
I also included a fix for the failing code coverage in this PR - see commit message for details |
Codecov Report
@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 0.48% 64.31% +63.82%
==========================================
Files 73 87 +14
Lines 6817 9429 +2612
Branches 1049 1553 +504
==========================================
+ Hits 33 6064 +6031
+ Misses 6777 2542 -4235
- Partials 7 823 +816
... and 79 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
These represent types which don't store any interesting data for us to measure, but which are required by a real container so can not be replaced with our own generated class types. std::allocator often has bad DWARF, so it must be replaced after the DWARF is fixed up in Flattener. The others could be replaced earlier in the transformation process if desired, but I've left them all together for simplicity for now. This fixes the folly::fbstring tests.
Multiple jobs are not allowed to persist the same files to a workspace. This commit takes the lazy approach of splitting the type-graph and non-type-graph jobs into different workspaces and creating a second coverage job to avoid the conflict.
ajor
force-pushed
the
allocator-pass-through
branch
from
ea1c442 to
30af9f1
Compare
| @@ -20,6 +20,15 @@ | |||
|
|
|||
| namespace type_graph { | |||
|
|
|||
| // TODO: | |||
| // - read these from a TOML file | |||
There was a problem hiding this comment.
can you create an issue for putting these in the config or creating container .tomls for them? I'd lean towards the config for simplicity but either works
There was a problem hiding this comment.
Agreed that just putting these in the config would be simpler - no need for a file for each
| @@ -20,6 +20,15 @@ | |||
|
|
|||
| namespace type_graph { | |||
|
|
|||
| // TODO: | |||
| // - read these from a TOML file | |||
| // - don't require specifying ctype and header | |||
There was a problem hiding this comment.
do you not need to specify the headers here? you could use "cstddef" in that case which is always included, but it looks to me like you do need the headers in weird edge cases
There was a problem hiding this comment.
Each of these dummy containers belongs to a real container and will only appear when that container is used. That means their header files will already be included by the real container.
Actually, typing this out has made me think that maybe "dummy container" should be a field in the container TOML of each real container that these belong to
There was a problem hiding this comment.
Oh, except std::allocator, which doesn't belong to any single container, but is used by all of them
Is 91.66% accurate? I'm amazed! |
We're at 100% coverage for the majority of type_graph files so I'd assume so! |
These represent types which don't store any interesting data for us to measure, but which are required by a real container so can not be replaced with our own generated class types.
std::allocator often has bad DWARF, so it must be replaced after the DWARF is fixed up in Flattener. The others could be replaced earlier in the transformation process if desired, but I've left them all together for simplicity for now.
This fixes the folly::fbstring tests.