Issue 37140 - new getters and setters #37645
Conversation
|
Review changes with |
|
gabc-cpp seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| /** | ||
| * Checks if readPrivilege has a value. | ||
| */ | ||
| bool readPrivilegeHasValue() const { return (mask & ATTRIBUTE_MASK_READ_PRVLG_ANY_VALUE); } |
There was a problem hiding this comment.
drop these: one can use GetReadPrivilege().has_value() or similar.
| // | ||
| // Read Privilege stored on the first trio of significant binary digits | ||
| // | ||
| // Attribute mask for any Read Privilege value set |
There was a problem hiding this comment.
Why are we doing this?
Privilege is already an almost bitflag object
// ProxyView, and Operate).
enum class Privilege : uint8_t
{
kView = 1 << 0,
kProxyView = 1 << 1,
kOperate = 1 << 2,
kManage = 1 << 3,
kAdminister = 1 << 4
};
Set is simple |= privilege << WriteorReadOffset
unset is also simple &= ~(privilege << WriteorReadOffset)
Clear is &= ~0b11111 << WriteorReadOffset
all these elements will end up in the binary bloating
and an unnecesary jumptable
There was a problem hiding this comment.
FYI there is a template BitMask<> that gives you set and unset an check for these types of enum values
There was a problem hiding this comment.
Access privilege is a bitmask to support a set, however specifically for attributes and commands it is not a set. So the difference would be using 5-6 bits vs 3. however the point seems valid: if we have a bitset that already works even if we use more bits, maybe we could use 6 bits for read, 6 for write and that leaves us with 20 bits worth of extra flags (and we need about 5 only right now)
| */ | ||
| Access::Privilege GetWritePrivilege() const | ||
| { | ||
| std::optional<Access::Privilege> writePrivilege; |
There was a problem hiding this comment.
Why create an optional when you are returning Access::Privilege?
There was a problem hiding this comment.
return should be an optional I believe - attributes may be read-only and we flag that via nullopt (no write privileges). Same for read.
For invoke however, commands always have some privileges so we return Access::Privilege instead of optional.
| /** | ||
| * @brief Type for the attribute mask | ||
| */ | ||
| typedef uint32_t AttributeMask; |
There was a problem hiding this comment.
If I understood the ticket right, we want to compress everything into a smaller structure, 3 bit for W/R Access each and the rest to some other for the other flags in the structures
an example being AttributeQualityFlags in Attribute entry
CommandQualityFlags in AcceptedCommandEntry etc...
Right now it is 8 bits for ReadPrivileges and 8 for WritePrivileges, 16 with this change you go to 32, not compacting at all
| /** | ||
| * Getter for readPrivilege. | ||
| */ | ||
| Access::Privilege GetReadPrivilege() const |
There was a problem hiding this comment.
You want to return the full bit flag not am incomplete one, like if I have View | ProxyView | Manage I will get only View privilege with this function
There was a problem hiding this comment.
You never have more than one bit set here in practice.
| /** | ||
| * Checks if writePrivilege has a value. | ||
| */ | ||
| bool writePrivilegeHasValue() const { return (mask & ATTRIBUTE_MASK_WRITE_PRVLG_ANY_VALUE); } |
| // Read Privilege stored on the first trio of significant binary digits | ||
| // | ||
| // Attribute mask for any Read Privilege value set | ||
| constexpr uint8_t ATTRIBUTE_MASK_READ_PRVLG_ANY_VALUE = 0x07; // == 0b00000111 |
There was a problem hiding this comment.
We shouldn't duplicate Privilege values....
| /** | ||
| * Checks if readPrivilege has a value. | ||
| */ | ||
| bool readPrivilegeHasValue() const { return (mask & ATTRIBUTE_MASK_READ_PRVLG_ANY_VALUE); } |
There was a problem hiding this comment.
Instead of doing manual bitops, have we considered just using a C++ bitfield here? So having:
uint16_t readPrivilege : 5;
uint16_t writePrivilege : 5;
and then just assign/read those members as normal?
There was a problem hiding this comment.
It sounds like an excellent solution, might want the set/get though to abstract whatever bitfield mechanism is being used
There was a problem hiding this comment.
Yes, the API should not be those raw bitfields.
| /** | ||
| * Getter for readPrivilege. | ||
| */ | ||
| Access::Privilege GetReadPrivilege() const |
There was a problem hiding this comment.
You never have more than one bit set here in practice.
| if ( (mask & ATTRIBUTE_MASK_READ_PRVLG_KADMINISTER_VALUE) == ATTRIBUTE_MASK_READ_PRVLG_KADMINISTER_VALUE ) | ||
| return (Access::Privilege::kAdminister); | ||
|
|
||
| return(Access::Privilege::kView); // generally defaults to View if readable |
There was a problem hiding this comment.
In theory we can have write-only attributes. None in the spec today, however unittestingcluster has one such thing. This is why we return optional here.
Issue 37140 - Make metadata that combines flags and access privileges more compact
Changes to the following files:
src/app/data-model-provider/MetadataTypes.h
Added bitmasks to handle the different values of
Access::Privilege.Added getters and setters for
readPriviligeandwritePrivilige.src/app/WriteHandler.cpp
src/app/reporting/Engine.cpp
src/data-model-providers/codegen/CodegenDataModelProvider.cpp
Added several call for the new setters and getters.
Additional setters and getters are needed for the framework backend used for several unit tests. Work in progress.