Fix counters attribute name #639
Conversation
|
I will commit soon an example where the extra indirection is actually good: we had changed default_only to defaultonly in the spec, but not in the code. A question you are raising is whether a table can have simultaneously both counters and direct counters; in that case you may need to use two different properties. |
|
Actually, maybe I am wrong, and counters do not need a table property at all. |
| @@ -400,7 +400,7 @@ ControlConverter::convertTable(const CFG::TableNode* node, | |||
| size = BMV2::TableAttributes::defaultTableSize; | |||
|
|
|||
| result->emplace("max_size", size); | |||
| auto ctrs = table->properties->getProperty(BMV2::TableImplementation::directCounterName); | |||
| auto ctrs = table->properties->getProperty(BMV2::TableAttributes::countersName); | |||
There was a problem hiding this comment.
Maybe we can just delete these from TableImplementation?
There was a problem hiding this comment.
This is the only change I am requesting: removal of the unused constants.
There was a problem hiding this comment.
Are you suggesting replacing them with string constant literals? Or something else?
|
Arguably we should reorganize this code to not look at the table properties at all, but instead just look at the extern method calls in actions on the table. |
|
In this case you will also need to do some more checks, e.g., that counters are not shared between different tables or that method calls only occur in actions. |
|
We need those checks regardless, independent of how we recognize which table(s) a counter or meter is attached to. |
ChrisDodd
force-pushed
the
cdodd
branch
3 times, most recently
from
7f0231c to
b1da71f
Compare
ChrisDodd
force-pushed
the
cdodd
branch
2 times, most recently
from
f825eaf to
f2030e4
Compare
|
I don't think we need both TableImplementation::directCounterName and TableATtributes::countersName. One of them should be just deleted. |
|
They're different -- TableImplementation::directCounterName is "direct_counter" and TableAttributes::countersName is "counters" -- the "counters" table attribute refers to a list of externs that must be either "direct_counter" or "counter" instances (TableImplementation::directCounterName or TableImplementation::counterName). Whether we need the "counters" attribute at all is another question -- or is mentioning the counters (with a method call in an action of the table) enough? At the moment we check that the method calls and the "counters" attribute are consistent, and give an error if they are not (I think). |
|
OK, then I guess what we need is this kind of documentation - that you just typed - in the code. |
Fix (partly) issue #461
A good example why obfuscating string constants behind
static constexpr"names" is a bad idea -- makes the code harder to read and maintain and makes it easier for bugs to creep in.Arguably we should add a check for
countcalls on counters that are not named in the table attributes. Whether that should be an error or should implicitly add said counter attribute is unclear.