On 2 March 2012 22:26, Jens Seidel <[email protected]> wrote: > Am 28. Februar 2012 11:58 schrieb Manuel A. Fernandez Montecelo > <[email protected]>: >> So what's technically wrong with this one: >> http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=c6e2914b0c751d591ad3e39743c7d417e7f6aaef > > + return theme_config->FindVector((themeroot+Name).c_str()); > > themeroot+Name is a temporary string which gets soon destroyed whereas > the pointer to it is passed to FindVector. >
Hi Jens That is certainly an issue to be aware of. In this case it is safe to call FindVector like this as the temporary string will remain allocated until the function returns. I have been using such temporaries extensively in recent changes. I did have the same concern as yourself when I started so I checked this out with a small test program first and found some confirmation[1] online (though did not manage to locate anything in C++ reference). [1] http://stackoverflow.com/questions/7581680/pass-c-str-return-value-of-temporary-object-to-printf >> That you changed to: >> http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=69deef1f4a9678a67a547b5dd8c35f6303832ce0 > > + return theme_config->FindVector(themeroot+Name); > + inline std::vector<string> FindVector(string Name) > > Here the temporary string is handled via call by value which makes a copy of > it. > This is save. > > But I agree that this fix is not described in the commit log so maybe > it was not noticed (or I'm wrong :-)) > The log was terse ;-) but did identify the single issue: consistency. This class contains approximately ten near-identical wrapper functions. The FindVector wrappers did not follow the same pattern as the rest of those functions making it non-trivial to see that they all perform the same task with the same constraints. The existing pattern has been functioning correctly for years. I agree that this could be updated to a more efficient and/or const-correct form. For the sake of maintainability surely all the functions should be updated together. Anyway, I thought it an obvious and easy change, so I just made it. Perhaps I overstepped the boundary; I'm not going to interfere with it being changed back at this point. _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/aptitude-devel
