Remove pub from AccountMapEntryMeta fields#8341
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8341 +/- ##
=========================================
- Coverage 83.2% 83.2% -0.1%
=========================================
Files 838 838
Lines 368481 368481
=========================================
- Hits 306709 306702 -7
- Misses 61772 61779 +7 🚀 New features to boost your workflow:
|
kskalski
force-pushed
the
ks/no_pub_meta
branch
from
fe099dd to
f000bb6
Compare
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
Overall I agree with removing pub from AccountMapEntry::meta.
I'm indifferent for AccountMapEntryMeta though. Since the struct itself is pub, probably safer to make its field private, so works for me.
| fn new(dirty: bool, age: Age) -> Self { | ||
| AccountMapEntryMeta { | ||
| dirty: AtomicBool::new(false), | ||
| age: AtomicAge::new(storage.future_age_to_flush(false)), | ||
| dirty: AtomicBool::new(dirty), | ||
| age: AtomicAge::new(age), | ||
| } |
There was a problem hiding this comment.
I'm actually mostly against constructors that just wrap struct initialization. Especially when they are private.
These changes seem unnecessary to just remove the pub from AccountMapEntry and AccountMapEntryMeta fields, at least. So the PR is now doing more stuff than I think is necessary.
There was a problem hiding this comment.
I'm adding constructors, because I want to add a field to meta, that callers shouldn't really (be forced to) initialize.
But I think the exact choice of how and where initialization happens actually changed after #8337, so your point makes sense.
There was a problem hiding this comment.
Mainly I think this PR could be three lines of removing pub and it would be an easy approval. Could add ctors in separate PRs that actually use/need them.
| ref_count: AtomicRefCount::default(), | ||
| meta: AccountMapEntryMeta::default(), | ||
| } | ||
| Self::new(SlotList::new(), 0, AccountMapEntryMeta::default()) |
kskalski
force-pushed
the
ks/no_pub_meta
branch
from
f000bb6 to
f554e34
Compare
4 participants
Problem
AccountMapEntryMetahas pub fieldsAccountMapEntryThis exposure is unnecessary
Summary of Changes
Remove pub attributes