[Aptitude-devel] Stepping down as maintainer/developer

[Daniel Hartwig]

On 2 March 2012 22:26, Jens Seidel <jensseidel at users.sf.net> wrote:
> Am 28. Februar 2012 11:58 schrieb Manuel A. Fernandez Montecelo
> <manuel.montezelo at gmail.com>:
>> 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.