Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take list of pass-through types from config instead of hardcoding #190

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

ajor
Copy link
Contributor

@ajor ajor commented Jun 28, 2023

As we now store ContainerInfo objects in OICodeGen::Config, we can not copy it any more. Change all places that took copies to take const references instead.

The copy in OICodeGen modified membersToStub, the contents of which form part of OICache's hash. However, as OICache also previously had its own copy, it would not have been OICodeGen's modifications.

std::set<std::string> defaultHeaders{};
std::set<std::string> defaultNamespaces{};
std::vector<std::pair<std::string, std::string>> membersToStub{};
std::set<fs::path> containerConfigPaths;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {} makes no difference on these members, as class-objects are always default initialised if not initialised explicitly.

However, the error messages are a lot clearer without the {} and removing them allowed me to track down all the locations OICodeGen was being copied.

@ajor ajor force-pushed the pass-through-types branch from bb18cb7 to 595c1dd Compare June 29, 2023 07:07
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #190 (a6105df) into main (6aead62) will increase coverage by 0.17%.
The diff coverage is 71.11%.

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   65.19%   65.37%   +0.17%     
==========================================
  Files          87       87              
  Lines        9429     9452      +23     
  Branches     1557     1560       +3     
==========================================
+ Hits         6147     6179      +32     
+ Misses       2459     2447      -12     
- Partials      823      826       +3     
Impacted Files Coverage Δ
oi/OIDebugger.h 43.13% <ø> (ø)
test/test_type_identifier.cpp 91.17% <0.00%> (ø)
oi/CodeGen.cpp 71.91% <33.33%> (+4.67%) ⬆️
oi/OIUtils.cpp 48.88% <53.33%> (+0.88%) ⬆️
oi/OIDebugger.cpp 38.08% <83.33%> (-0.05%) ⬇️
oi/CodeGen.h 100.00% <100.00%> (ø)
oi/OICache.h 80.00% <100.00%> (+30.00%) ⬆️
oi/OICodeGen.cpp 67.11% <100.00%> (ø)
oi/OICodeGen.h 93.33% <100.00%> (+0.47%) ⬆️
oi/OID.cpp 32.38% <100.00%> (+0.21%) ⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ajor ajor force-pushed the pass-through-types branch from 595c1dd to 780207b Compare June 29, 2023 10:57
oi/OICodeGen.h Outdated
Comment on lines 60 to 63
/*
* Note: don't set default values for the config so the user gets an
* uninitialized field" warning if they missed any.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ttreyer I don't understand what exactly this comment is getting at. I've tried a few things and can't seem to get an uninitialised field warning

@ajor ajor requested a review from ttreyer June 29, 2023 12:14
@ajor ajor force-pushed the pass-through-types branch from 780207b to a6105df Compare July 3, 2023 12:48
@@ -47,3 +47,8 @@ containers = [
"PWD/types/thrift_isset_type.toml",
"PWD/types/weak_ptr_type.toml",
]
pass_through = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which README.md do we document these options in? I can't see anything in this diff but swore we docuemnted them somewhere (not the toml container stuff).

Copy link
Contributor Author

@ajor ajor Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't documented yet.

We've got READMEs for the container types config format and for the integration test format, nothing else so far

Copy link
Contributor

@tyroguru tyroguru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great Alastair. Just one point about documenting the new option which I'm sure you've got covered anyway.

As we now store ContainerInfo objects in OICodeGen::Config, we can not
copy it any more. Change all places that took copies to take const
references instead.

The copy in OICodeGen modified membersToStub, the contents of which form
part of OICache's hash. However, as OICache also previously had its own
copy, it would not have been OICodeGen's modifications.
@ajor ajor force-pushed the pass-through-types branch from a6105df to 7a9decb Compare July 5, 2023 12:26
@ajor
Copy link
Contributor Author

ajor commented Jul 5, 2023

Thanks, yeah we will document this file at some point, but I'll leave it for a separate PR

@ajor ajor merged commit 0330ef8 into facebookexperimental:main Jul 5, 2023
@ajor ajor deleted the pass-through-types branch July 5, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants