> On March 2, 2014, 11:01 p.m., Matt Jordan wrote: > > /trunk/include/asterisk/devicestate.h, lines 278-281 > > <https://reviewboard.asterisk.org/r/3281/diff/1/?file=55014#file55014line278> > > > > I'm certainly fine with losing the cache_id, as it is no longer > > necessary. > > > > I'm not fan of changing the stringfield to a const char *, simply > > because it really is non-const, and you have to cast the const char * back > > to a char * to free it. Generally, if something is const it should always > > be const; if not, well, it's not. > > > > Granted, string fields with a single entry a bit goofy, but I think I'd > > rather go with that goofiness over the const gyrations. > > rmudgett wrote: > The device name string is immutable throughout its lifetime. It is just > malloced by ast_strdup() to hold the value. It is a quirk of the free() > function signature that it takes a non-const pointer and thus requires the > cast to get rid of the memory. > > Stringfield strings are declared as const when they are used. You have > to call a function to set a new value. > > typedef const char * ast_string_field; > #define AST_STRING_FIELD(name) const ast_string_field name > > > Matt Jordan wrote: > I still think this makes the semantics of the field more difficult to > understand, not less. I would generally not assume I need to free the thing > pointed to by a const char *, as the value cannot be modified by my pointer. > Your approach seems to violate that intent. > > I'm not sure how this change adds any value over the string field > approach. With a string field, you would initialize it and set it in the > allocation routine. You would de-init the string field in the object's ao2 > destructor. That doesn't appear to be any more work than what you have here, > and the usage semantics are clear. > > rmudgett wrote: > You're adding meaning where it is not implied. A const pointer doesn't > mean that what it points to is not allocated. It just means that you cannot > change its contents using that pointer. In this case, the alloc and free are > encapsulated inside the device_state_alloc()/device_state_dtor() functions > respectively. Nothing else cares where it is stored. > > Maybe I should change the alloc function to store the device and eid > field values after the struct. That would get rid of the destructor function > entirely. > > String fields add a lot of overhead for one string. String fields have > three management pointers plus a pointer for each string. You don't really > start getting a benefit from the overhead until you have three or more > strings. >
I'll grant that what you are doing is not outsides of what is const, but it is a pattern that is not used in Asterisk. matt@mjordan-laptop:~/projects/12$ grep -r --include=*.c "ast_free((char*)" . matt@mjordan-laptop:~/projects/12$ I'd prefer to not introduce a slippery slope of const casting. That way lies madness, even if it is technically correct in this case. - Matt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3281/#review11018 ----------------------------------------------------------- On March 1, 2014, 7:07 p.m., rmudgett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3281/ > ----------------------------------------------------------- > > (Updated March 1, 2014, 7:07 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23204 > https://issues.asterisk.org/jira/browse/ASTERISK-23204 > > > Repository: Asterisk > > > Description > ------- > > A stasis cache entry now contains more than a single message/snapshot. It > contains messages/snapshots for the internal entity as well as any external > entities that post to the cached item. In addition callbacks can be supplied > when the cache is created to compute and post the aggregate message/snapshot > representing all entities stored in the cache entry. > > * All stasis messages now have an eid to indicate what entity posted it. > > * The stasis cache enhancements allow device state to cache and aggregate the > device states from internal and external entities in a single operation. The > cached aggregate device state is available immediately after it is posted to > the stasis bus. This improves performance by eliminating a cache dump and > associated ao2 container traversals to calculate the aggregate state. > > > Diffs > ----- > > /trunk/tests/test_devicestate.c 409306 > /trunk/main/stasis_message.c 409306 > /trunk/main/stasis_cache.c 409306 > /trunk/main/devicestate.c 409306 > /trunk/main/app.c 409306 > /trunk/include/asterisk/stasis.h 409306 > /trunk/include/asterisk/devicestate.h 409306 > > Diff: https://reviewboard.asterisk.org/r/3281/diff/ > > > Testing > ------- > > All unit tests pass including the stasis and device state tests. > > The device state unit test had to be changed to get the aggregate state out > of the new cache location. Fortunately, the normal users of the device state > aggregate information subscribe to the events and don't get the aggregate > device state out of the cache. > > > Thanks, > > rmudgett > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
