FilterState: WriteOnce and WriteMany support#4820
Conversation
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
| * immutable. This function will fail if the data stored under | ||
| * |data_name| cannot be dynamically cast to the type specified. | ||
| */ | ||
| template <typename T> T& getData(absl::string_view data_name) { |
There was a problem hiding this comment.
Call to non-const ref/pointer of FilterState will be go here regardless of the data_name (and an exception will be thrown for immutable data), so this should be named differently than the const one above, name this mutableData?
There was a problem hiding this comment.
So the LHS doesn't matter at all?
const auto& foo = filterState.getData() ?
There was a problem hiding this comment.
No, unless you cast filterState to const. i.e. static_cast<const FilterState&>(filterState).getData()
…te_many_filter_state Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
…te_many_filter_state Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
|
I am mainly looking for feedback on whether these interfaces are acceptable. Will add the tests etc. once we agree on the interfaces. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
At a high-level this seems to match what we discussed last week, so it makes sense to me. I think @curiouserrandy might have a better reading of the interface changes.
| data_storage_[name] = std::move(data); | ||
|
|
||
| std::unique_ptr<FilterStateImpl::FilterObject> filter_object; | ||
| filter_object.reset(new FilterStateImpl::FilterObject()); |
There was a problem hiding this comment.
nit: = make_unique and combine with one line above.
…te_many_filter_state Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
| */ | ||
| virtual void setData(absl::string_view data_name, std::unique_ptr<Object>&& data) PURE; | ||
| virtual void setData(absl::string_view data_name, std::unique_ptr<Object>&& data, | ||
| StateType state_type) PURE; |
There was a problem hiding this comment.
My personal preference would be to have two separate interfaces rather than passing in an (effective) boolean (I think it'd be harder to mis-use and easier to audit if a company wanted to forbid one of them) but I am not dead-set on the idea.
There was a problem hiding this comment.
I was hoping the enum would make things cleaner
There was a problem hiding this comment.
I think it's equally clear if we have setDataImmutable and setDataMutable vs. having the enum. So, I'm on the fence on this one. The enum does support more dynamic reflective behavior, but I don't think that's a strong selling point here. The getters don't use an enum... should we be consistent in naming and signatures?
…te_many_filter_state Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Interface LGTM. Nice! Will defer to @htuch for complete review.
| */ | ||
| class FilterState { | ||
| public: | ||
| enum StateType { ReadOnly, Mutable }; |
…te_many_filter_state Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo setter vs. getter consistency comment.
| */ | ||
| virtual void setData(absl::string_view data_name, std::unique_ptr<Object>&& data) PURE; | ||
| virtual void setData(absl::string_view data_name, std::unique_ptr<Object>&& data, | ||
| StateType state_type) PURE; |
There was a problem hiding this comment.
I think it's equally clear if we have setDataImmutable and setDataMutable vs. having the enum. So, I'm on the fence on this one. The enum does support more dynamic reflective behavior, but I don't think that's a strong selling point here. The getters don't use an enum... should we be consistent in naming and signatures?
|
The getreadonly can get both mutable and immutable (so enum doesn't help here - stateType::ReadOnly looks odd). Also, setReadOnly and getReadOnly are not counter parts because I can get mutable as a readOnly object. The enum leaves the door open for more types in future IMO. |
5 participants
Based on the discussions in this design doc:
https://docs.google.com/document/d/101Rcggh7flniYKgsr2T07yHXApywbnXiKj1hIpxX6yQ/edit#
Adds support for storing/retrieving mutable filter state objects, in addition to existing support for immutable filter state objects.
Signed-off-by: Shriram Rajagopalan shriramr@vmware.com