On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 06:18 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
I know, and I see how it looks insanely smart and simple today. Until you realize that the amazing "copy_filename" function doesn't touch the target at all if it gets passed in NULL. And all of that implicitly. So implicitly it will leave the old value in the filename if nothing is set in env.
I think you are mis-reading the code. If src is NULL, it will set dst[0] = '\0'; I think the behavior is quite reasonable.
Imaging you're trying to read the code 4 years from now. Will you remember all of the side effects of copy_filename()? Imagine you're someone who's new to the code and doesn't know all the implicit side effects of its functions. Will you understand what is going on at a glimpse?
That's an argument for documentation... and anyway, yes, I think the function is sensible and I would expect it to do what it does.
-Joe