<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 30, 2014 at 5:59 AM, Joshua Colp <span dir="ltr"><<a href="mailto:jcolp@digium.com" target="_blank">jcolp@digium.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">Matthew Jordan wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br></blockquote></div></blockquote><div><br></div><div><snip> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote></div><div class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
:: Setting Attributes ::<br>
<br>
The biggest sticky point is clearly #1. I don't think we should lose the<br>
immutability convention - that feels bad, since there's a lot of benefit<br>
to treating a format as a read-only object (no locking!) - and a format<br>
shouldn't change very often. In order to maintain mutability, I'd remove<br>
the public format attribute setting function and instead do the following:<br>
</blockquote>
<br></div>
I disagree with this. Having the ability to set via name/value attributes could be extremely useful in the future. For example it would easily allow a module to exist which allows you to add to the cached formats of the system, with attributes. This is something that wasn't possible before. For simple things it would even take care of SDP.</blockquote>
<div><br></div><div>If we allow attributes to be set after format creation, we have two options:<br><br></div><div>(1) We make formats mutable, and all the badness that entails.<br><br></div><div>(2) Setting a format implicitly dupes the existing format, adds the new attribute, then returns a new format.<br>
<br></div><div>Going with option #2 (mutable == bad), I'm not sure how useful this is as a public operation. When you get an SDP, the attributes will be presented to you as a big ol' blob of text. You don't want to parse those outside of something that is creating the format - otherwise you'll end up with this incredibly bad code:<br>
<br>format1 = ast_format_attribute_set(format, "foo", "bar");<br></div><div>format2 = ast_format_attribute_set(format1, "yackity", "schmackity");<br></div><div>format3 = ast_format_attribute_set(format2, "oh_nos", "the_pain");<br>
</div><div>ao2_cleanup(format2);<br>ao2_cleanup(format1)<br>ao2_cleanup(format);<br></div><div>format = format3;<br><br></div><div>Ew.<br><br></div><div>This, on the other hand, is slightly cleaner:<br><br></div><div>new_format = ast_format_sdp_parse(format, my_sdp_with_attributes);<br>
</div><div>ao2_cleanup(format);<br></div><div>format = new_format;<br></div><div><br></div><div>And is how both chan_sip and res_pjsip_sdp_rtp operate.<br></div><div><br></div><div>While we may some day want to add a public function that adds attributes... YAGNI? <br>
</div><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
:: Getting Attributes ::<br>
<br>
Since the format is read-only, it's safe to get the format attributes<br>
from a format object. There is one place where this is done using<br>
ast_format_isset. ast_format_get_value is never used. As the object is<br>
immutable, a public accessor for an attribute key could be used that<br>
returns a const char *, which would fulfil both functions.<br>
</blockquote>
<br></div>
The design currently gets away from the whole attribute key thing. Attributes are stored in whatever way the format interface has decided via an AO2 structure associated with the format itself. It needs a way to set/get that.<div class="HOEnZb">
<div class="h5"><br>
<br></div></div></blockquote><div><br></div><div>How so? The current design calls for a public function that accepts a const char *key and a const char *value. The key and value are always going to be presented to the core as text (inbound SDP), and typically will be generated as text (outgoing SDP). Yes, sometimes we may need to interpret the values as integers, or floats, or something - but that will typically be the domain of the resource modules. I'm not sure storing them as heterogeneous types is all that advantageous.<br>
<br clear="all"></div></div><br>-- <br><div dir="ltr"><div>Matthew Jordan<br></div><div>Digium, Inc. | Engineering Manager</div><div>445 Jan Davis Drive NW - Huntsville, AL 35806 - USA</div><div>Check us out at: <a href="http://digium.com" target="_blank">http://digium.com</a> & <a href="http://asterisk.org" target="_blank">http://asterisk.org</a></div>
</div>
</div></div>