[vlc-devel] [PATCH v2 4/13] esout: refactor GetTmpFile()

[Lyndon Brown]

On Wed, 2020-10-07 at 20:55 +0300, Rémi Denis-Courmont wrote:
> Le keskiviikkona 7. lokakuuta 2020, 4.14.01 EEST Lyndon Brown a écrit
> :
> > On Tue, 2020-10-06 at 23:40 +0300, Rémi Denis-Courmont wrote:
> > > Le tiistaina 6. lokakuuta 2020, 23.31.36 EEST Rémi Denis-Courmont 
> > > a
> > > 
> > > écrit :
> > > > How about *actually* factoring the admittedly repetitive code?
> > > > 
> > > > You can just call GetTempFile(filename, "/tmp"); and divide the
> > > > length of
> > > > the function in half. This does not need any external change.
> > > 
> > > And in fact, we really don't need a file name here at all,
> > > because we
> > > really
> > > don't need two concurrent file handles to the same file.
> > > 
> > > Could just as well use tmpfile() which would be safe from leaking
> > > disk space.
> > 
> > I did make a little effort to trace the call stack of the function
> > to
> > better understand its use when I made the change, but did not go
> > all
> > the way with it.
> 
> It's just opening one FILE pointer for writing and another one for
> (seeking 
> and) reading. The whole point of temporary files is to open them
> read/write and 
> seek.
> 
> > If this is so, and no doubt you're right, then we should then
> > consider
> > dropping patch #9 since this function is the only user of the extra
> > helper param that introduces.
> 
> Yes.
> 
> Are you planning to do it though?

Yes of course. All I need are responses from you wrt. our discussion of
the last two patches so I know precisely what you want me to do there.

I'll look into changing this function as you advise (which sounds
perfectly simple enough), tweak things per the other review comments,
adjust the last two as you suggest, and publish a v3.