On Mon, Feb 24, 2020 at 12:39:49PM +0100, Enrico Forestieri wrote:
> On Sun, Feb 23, 2020 at 08:41:12PM -0500, Scott Kostyshak wrote:
> > On Sun, Feb 23, 2020 at 10:40:32PM +0100, Enrico Forestieri wrote:
> > > On Sun, Feb 23, 2020 at 12:04:20PM -0500, Scott Kostyshak wrote:
> > > > On Sun, Feb 23, 2020 at 03:10:37PM +0100, Enrico Forestieri wrote:
> > > > > On Sun, Feb 23, 2020 at 08:22:55AM -0500, Scott Kostyshak wrote:
> > > > > > On Wed, Feb 19, 2020 at 08:07:46PM +0100, Enrico Forestieri wrote:
> > > > > > > On Wed, Feb 19, 2020 at 01:19:54PM -0500, Scott Kostyshak wrote:
> > > > > > > >
> > > > > > > > It seems I committed too soon. Sorry for not waiting. Both the
> > > > > > > > macro
> > > > > > > > approach and Enrico's proposal are cleaner than my approach. I
> > > > > > > > was
> > > > > > > > planning to pursue the macro approach in a follow-up commit.
> > > > > > >
> > > > > > > Apparently, the macro approach was abandoned by the Qt folks.
> > > > > > >
> > > > > > > > Regarding
> > > > > > > > C++11, don't we already use range-based for loops? Or is the
> > > > > > > > question
> > > > > > > > about if we require *all* of C++11?
> > > > > > >
> > > > > > > The latter. As shown by Pavel in the other post gcc 4.7 is lacking
> > > > > > > something. As we use xcb_selection_notify_event_t only in one
> > > > > > > place,
> > > > > > > I think defining a macro is overkill. In order to avoid many calls
> > > > > > > to calloc() (I don't know how memory fragmentation is dealt with
> > > > > > > by
> > > > > > > modern compilers), we could anyway use that idea as follows:
> > > > > > >
> > > > > > > union {
> > > > > > > xcb_selection_notify_event_t event;
> > > > > > > char padding[32];
> > > > > > > } padded_event;
> > > > > > > auto & nev = padded_event.event;
> > > > > >
> > > > > > Enrico, I propose that you commit. Thanks for the fix.
> > > > >
> > > > > According to the followups to the post by Pavel and commit 748bb5a0,
> > > > > I think that the C++11 alignas solution is preferred. Given that you
> > > > > can check its effectiveness, I think that it is better that you
> > > > > commit it.
> > > >
> > > > I tried to test but probably I'm missing something in the change that I
> > > > tested since I still get the following with Valgrind:
> > >
> > > Please, can you also try with the union approach above?
> >
> > I tested but still get the Valgrind error. I'm wondering if I
> > implemented the approach incorrectly. Attached is the patch.
>
> No, you are doing it right. This seems to be a limitation of valgrind
> that, seemingly, does not take into account that nev is part of a large
> enough union.Isn't it correct that not all of the bytes in the union are initialized? It is just that in this case it is not a problem if those bytes get written to later on by xcb_flush, but Valgrind cannot know that. We could apply the union patch and suppress the error in Valgrind with the --suppressions option. I've been planning to write a Valgrind suppressions file for LyX and post it somewhere (not sure if in the repo or on the Wiki). I was hoping to avoid this for a while though since I'm tired of using Valgrind because it is so darn slow (although I do not blame it since it has to track so many things). Apparently another approach would be to add the following: memset(&padded_event, 0, sizeof(padded_event)); Valgrind does not complain when this line is added to the union patch. I was wondering why the calloc approach does not give an error but after searching it seems that calloc zero-initializes the memory. Off-topic: I'm hoping to read this book someday to learn more about these topics: https://www.amazon.com/Under-Hood-Anthony-Dos-Reis/dp/1793302898 Scott
signature.asc
Description: PGP signature
-- lyx-devel mailing list lyx-devel@lists.lyx.org http://lists.lyx.org/mailman/listinfo/lyx-devel
