I found that BusTransation exported an interal API bus_transaction_get_connections() which in fact do bus_context_get_connections(). However, there is a DBusList *connections member in BusTransation, so it's somehow a little confusing why bus_transaction_get_connections() return the context->connections rather than transaction->connections. And infact, the two places invoked bus_transaction_get_connections() can be replaced by bus_context_get_connections() directly. More clearly and one call frame lesser. Giving that this is a internal API, so I think it's safe to change.
Created attachment 89181 [details] [review] [PATCH] BusTransaction: returns its own connections through getter
Can the two lists ever be different? For instance, it might be the case that one of them contains "completed" connections, and the other also includes connections that have not yet authenticated.
(In reply to comment #2) > Can the two lists ever be different? For instance, it might be the case that > one of them contains "completed" connections, and the other also includes > connections that have not yet authenticated. (In reply to comment #2) > Can the two lists ever be different? For instance, it might be the case that > one of them contains "completed" connections, and the other also includes > connections that have not yet authenticated. This patch doesn't introduce any behaviour change, just make the code (internal API) a little less confusing. In fact, yes they are different, one is a pointer of DBusList while the other one is a pointer of BusConnections. :-) Since we can use a more clear API bus_context_get_connections() to get context->connections, I think it's better to make bus_transaction_get_connections() to return its own connections, a pointer to DBusList.
(In reply to comment #3) > (In reply to comment #2) > > Can the two lists ever be different? For instance, it might be the case that > > one of them contains "completed" connections, and the other also includes > > connections that have not yet authenticated. > > This patch doesn't introduce any behaviour change, just make the code > (internal API) a little less confusing. > > In fact, yes they are different, one is a pointer of DBusList while the > other one is a pointer of BusConnections. :-) BusConnection contains two DBusList<DBusConnection> lists, "completed" and "incomplete". BusTransaction.connections could reasonably correspond to completed, or the union of completed and incomplete, or something else entirely (just the connections involved in the transaction, maybe). From a skim-read, it looks like it's actually the connections involved in the transaction. > Since we can use a more clear API bus_context_get_connections() to get > context->connections, I think it's better to make > bus_transaction_get_connections() to return its own connections, a pointer > to DBusList. If bus_transaction_get_connections() currently returns the complete set of connections, and none of its callers actually need access to BusTransaction.connections, then I'd prefer to delete bus_transaction_get_connections() entirely (replacing it with the equivalent calls to bus_context_get_connections), rather than changing its semantics to something nobody needs.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Can the two lists ever be different? For instance, it might be the case that > > > one of them contains "completed" connections, and the other also includes > > > connections that have not yet authenticated. > > > > This patch doesn't introduce any behaviour change, just make the code > > (internal API) a little less confusing. > > > > In fact, yes they are different, one is a pointer of DBusList while the > > other one is a pointer of BusConnections. :-) > > BusConnection contains two DBusList<DBusConnection> lists, "completed" and > "incomplete". BusTransaction.connections could reasonably correspond to > completed, or the union of completed and incomplete, or something else > entirely (just the connections involved in the transaction, maybe). From a > skim-read, it looks like it's actually the connections involved in the > transaction. > > > Since we can use a more clear API bus_context_get_connections() to get > > context->connections, I think it's better to make > > bus_transaction_get_connections() to return its own connections, a pointer > > to DBusList. > > If bus_transaction_get_connections() currently returns the complete set of > connections, and none of its callers actually need access to > BusTransaction.connections, then I'd prefer to delete Yes, as you can see, no one needs BusTransaction.connections. > bus_transaction_get_connections() entirely (replacing it with the equivalent > calls to bus_context_get_connections), rather than changing its semantics to > something nobody needs. Hmm, I have no objection to do this, both of them are fine to me, a unused getter for BusTransaction.connections or delete it. Anyway, I'll upload a v2 which delete it totally.
Created attachment 89929 [details] [review] [PATCH] BusTransaction: remove confusing getter of connections deleted unused bus_transaction_get_connections()
Fixed in git for 1.7.10
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.