Re: [PATCH wayland 1/4] scanner: use c99 initializers for the wl_interface * array
Hi Emil, Discussing the scanner symbol visibility patch made me realise that I never replied here. On 15 September 2017 at 15:39, Pekka Paalanen wrote: > On Wed, 30 Aug 2017 16:33:15 +0100 Emil Velikov > wrote: >> Allows us to remove the explicit NULL init, keeping the list shorter and >> easier to read. > > looks like all of these patches change the scanner output in some way, > but none of them fix the corresponding tests. You'll see it if you run > 'make check'. We expect scanner patches to fix the expected test output > data in the same go, so that the tests keep on passing. The reason for > this is that we can see immediately from the patch itself how it > changes the scanner output without having to manually diff to verify. > > It's easy to fix: run 'make check' and copy the actual output over the > expected output and git-add. Ensure 'make check' now succeeds. The > paths are in tests/scanner-test.sh.log. This would definitely need fixing before we could do anything. But that being said, my worry is that we don't actually control the compilation environment for the scanner output. Scanner output currently compiles with '-pedantic -ansi -Wall -Wextra' (at least, when inline is defined). This patch changes that requirement, and I worry that - like previous discussions on changing scanner output - that upgrading Wayland would lead to people hitting compilation failures. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/4] scanner: use c99 initializers for the wl_interface * array
On Wed, 30 Aug 2017 16:33:15 +0100 Emil Velikov wrote: > On 30 August 2017 at 16:21, Emil Velikov wrote: > > From: Emil Velikov > > > > Allows us to remove the explicit NULL init, keeping the list shorter and > > easier to read. > > Hi Emil, looks like all of these patches change the scanner output in some way, but none of them fix the corresponding tests. You'll see it if you run 'make check'. We expect scanner patches to fix the expected test output data in the same go, so that the tests keep on passing. The reason for this is that we can see immediately from the patch itself how it changes the scanner output without having to manually diff to verify. It's easy to fix: run 'make check' and copy the actual output over the expected output and git-add. Ensure 'make check' now succeeds. The paths are in tests/scanner-test.sh.log. Patch 1 sounds like a nice idea. It also makes it easier to see which row a 'types + N' refers to. I'll wait to see the diff on the test output though. Patches 2, 3 and 4 miss the commit message explaining why you wrote them. I guess it's the same: easier to read. I have to say the only time I actually read those generated files is when I review scanner changes. About the DEFINE_INTERFACE(zxdg_surface_v6, 1) idea, I don't know what the benefit would be. No-one writes it by hand, the generator handles the repetitive writing anyway. IMHO, the code is more obvious without such a macro. > Note: > As the trailing NULL entries will be omitted, the actual array size > will be smaller as-is. > Not sure if/how much that would matter as neither requests or events > would reference those. > > Still making it a fixed size array might be the wiser move. Fixed size sounds good. It does look like libwayland wouldn't try to access the trailing NULLs, but I'd feel safer keeping them. Thanks, pq pgpGFuKUBN7P9.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/4] scanner: use c99 initializers for the wl_interface * array
On 30 August 2017 at 16:21, Emil Velikov wrote: > From: Emil Velikov > > Allows us to remove the explicit NULL init, keeping the list shorter and > easier to read. > Note: As the trailing NULL entries will be omitted, the actual array size will be smaller as-is. Not sure if/how much that would matter as neither requests or events would reference those. Still making it a fixed size array might be the wiser move. -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
