Re-order functions in stats_impl to group classes together#4004
Re-order functions in stats_impl to group classes together#4004mattklein123 merged 14 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
This reverts commit 769be4f. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ther. Signed-off-by: Joshua Marantz <jmarantz@google.com>
….cc. And un-inline much of IsolatedStoreImpl, to avoid having to recompile its large ctor with every other file in Envoy. Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
note to reviewers: this just moves functions around -- and un-inlines much of IsolatedStatsStore.
There should be no functional changes.
|
@jmarantz if you are going to do a code move PR would you prefer to split the files? Up to you. I could see it definitely helping with readability. |
|
sure -- np. Hold off on the review please; I'll split them up. Splitting the include will likely greatly expand the # of files in the CL but I'll keep it non-functional, so hopefully it'll still be easy to review. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… to its own file. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
That was definitely a lot more work. This change is again entirely non-functional; just moving stuff around. A fast review would be good here as this PR is highly prone to master merge conflicts. If you want to suggest changes to this org -- and they are not too pervasive -- it would also be better to do those as a follow-up; again due to potential issues with a delayed master merge. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this is awesome. Agreed we can do followups in other PRs.
2 participants
Description: stats_impl.cc/h is actually about 4 or 5 different classes which are not that tightly coupled. For include minimalism and being able to find what you are looking for easily, this PR breaks them up:
heap_stat_data.h metric_impl.h stats_impl.h tag_producer_impl.h
histogram_impl.h raw_stat_data.h stats_options_impl.h thread_local_store.h
isolated_store_impl.h stat_data_allocator.h tag_extractor_impl.h utility.h
There are now 1 or 2 tightly related classes per file.
Follow-up PRs are needed to break up stats_impl_test.cc and to rename stats_impl.h to stats_store.h, changing the 60+ refs to that.
Follow-up to #3710
Risk Level: Low
Testing: //test/...
Docs Changes: N/A
Release Notes: N/A