Yaron Keren via llvm-dev
2015-Aug-13 14:03 UTC
[llvm-dev] SmallString + raw_svector_ostream combination should be more efficient
Ok, one more issue. I've removed the no-op resync() method and now getting
rid of numerous raw_ostream::flush() calls. These were required before but
now will never do anything but will waste runtime discovering that.
To find raw_svector_ostream::flush() calls speficially I locally shadowed
raw_ostream:flush() with a deleted flush():
class raw_svector_ostream {
...
public:
...
void flush() = delete;
};
so any direct use of raw_svector_ostream::flush() fails compilation.
Calling raw_ostream:flush() itself would never reach the deleted function
as it's not virtual, it will continue to function as before.
To explicitly prevent calling raw_svector_ostream::flush(), is that a good
solution to commit?
2015-08-13 16:53 GMT+03:00 Rafael Espíndola <rafael.espindola at
gmail.com>:
> On 13 August 2015 at 04:17, Yaron Keren <yaron.keren at gmail.com>
wrote:
> > r244870 (with the change you requested), thanks!
> >
> > I initially placed the virtual function in header since they were
> > one-liners.
> > The coding standards say that an anchor() function should be supplied,
> which
> > was indeed missing. Other than required anchor function, why should
the
> > virtual functions go in the .cpp?
>
> Just because there is no advantage for most virtual methods to be in the
> .h.
>
> > Should I move the empty raw_svector_ostream destructor to the .cpp
file
> too
> > as well?
>
> Maybe. Destructors are the one exception since base class destructors
> call them non-virtually. If the class is final we can move them.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150813/18664a1f/attachment.html>
David Blaikie via llvm-dev
2015-Aug-13 17:56 UTC
[llvm-dev] SmallString + raw_svector_ostream combination should be more efficient
On Thu, Aug 13, 2015 at 7:03 AM, Yaron Keren via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Ok, one more issue. I've removed the no-op resync() method and now getting > rid of numerous raw_ostream::flush() calls. These were required before but > now will never do anything but will waste runtime discovering that. > To find raw_svector_ostream::flush() calls speficially I locally shadowed > raw_ostream:flush() with a deleted flush(): > > class raw_svector_ostream { > ... > public: > ... > void flush() = delete; > }; > > so any direct use of raw_svector_ostream::flush() fails compilation. > Calling raw_ostream:flush() itself would never reach the deleted function > as it's not virtual, it will continue to function as before. > > To explicitly prevent calling raw_svector_ostream::flush(), is that a good > solution to commit? >Seems reasonable to me - an alternative (that's probably worse) might be "using raw_ostream::flush" in the private section of raw_svector_ostream.> > > > 2015-08-13 16:53 GMT+03:00 Rafael Espíndola <rafael.espindola at gmail.com>: > >> On 13 August 2015 at 04:17, Yaron Keren <yaron.keren at gmail.com> wrote: >> > r244870 (with the change you requested), thanks! >> > >> > I initially placed the virtual function in header since they were >> > one-liners. >> > The coding standards say that an anchor() function should be supplied, >> which >> > was indeed missing. Other than required anchor function, why should the >> > virtual functions go in the .cpp? >> >> Just because there is no advantage for most virtual methods to be in the >> .h. >> >> > Should I move the empty raw_svector_ostream destructor to the .cpp file >> too >> > as well? >> >> Maybe. Destructors are the one exception since base class destructors >> call them non-virtually. If the class is final we can move them. >> >> Cheers, >> Rafael >> > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org http://llvm.cs.uiuc.edu > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150813/bddcfe57/attachment.html>
Yaron Keren via llvm-dev
2015-Aug-13 18:55 UTC
[llvm-dev] SmallString + raw_svector_ostream combination should be more efficient
Thanks, r244928 (clang + llvm), r244935+6 (lldb). 2015-08-13 20:56 GMT+03:00 David Blaikie <dblaikie at gmail.com>:> > > On Thu, Aug 13, 2015 at 7:03 AM, Yaron Keren via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Ok, one more issue. I've removed the no-op resync() method and now >> getting rid of numerous raw_ostream::flush() calls. These were required >> before but now will never do anything but will waste runtime discovering >> that. >> To find raw_svector_ostream::flush() calls speficially I locally shadowed >> raw_ostream:flush() with a deleted flush(): >> >> class raw_svector_ostream { >> ... >> public: >> ... >> void flush() = delete; >> }; >> >> so any direct use of raw_svector_ostream::flush() fails compilation. >> Calling raw_ostream:flush() itself would never reach the deleted function >> as it's not virtual, it will continue to function as before. >> >> To explicitly prevent calling raw_svector_ostream::flush(), is that a >> good solution to commit? >> > > Seems reasonable to me - an alternative (that's probably worse) might be > "using raw_ostream::flush" in the private section of raw_svector_ostream. > > >> >> >> >> 2015-08-13 16:53 GMT+03:00 Rafael Espíndola <rafael.espindola at gmail.com>: >> >>> On 13 August 2015 at 04:17, Yaron Keren <yaron.keren at gmail.com> wrote: >>> > r244870 (with the change you requested), thanks! >>> > >>> > I initially placed the virtual function in header since they were >>> > one-liners. >>> > The coding standards say that an anchor() function should be supplied, >>> which >>> > was indeed missing. Other than required anchor function, why should >>> the >>> > virtual functions go in the .cpp? >>> >>> Just because there is no advantage for most virtual methods to be in the >>> .h. >>> >>> > Should I move the empty raw_svector_ostream destructor to the .cpp >>> file too >>> > as well? >>> >>> Maybe. Destructors are the one exception since base class destructors >>> call them non-virtually. If the class is final we can move them. >>> >>> Cheers, >>> Rafael >>> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org http://llvm.cs.uiuc.edu >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150813/67b6e54d/attachment.html>