On Sep 13, 2018, at 3:49 AM, Frédéric Wang <fwang@igalia.com> wrote:

Hi,

I've recently found two non-trivial issues with unified builds. They are
not missing #include but instead conflicts between C++ files:

- Two C++ files including the same header but with a #define directive
set to different values [1].
- One C++ file including a header with implicit template instantiation
in an inline function before an explicit template specialization in a
second C++ file. [2]

I guess there could be more examples like this (e.g. functions with
conflicting names in two C++ files...). Should we allow to prevent some
files to be included in unified build? This is possible in Mozilla's
build system for example [3].

Or should we always try to re-write the code to fix the conflicts
between files? This might go against our usual practice: For example the
current solution I have for [2] is to move the function from the header
to its implementation C++ file, losing potential performance benefit of
inlining...

[1] https://bugs.webkit.org/show_bug.cgi?id=189579

^ Instead of your change here, I think the right fix is to make sure all files have the same value for IOSURFACE_CANVAS_BACKING_STORE. If headers depend on an #ifdef and different files have it set differently, that is a potential problem even without unified builds. Not in this case since the only difference is a static member function, but if the define affected data members, then it would be real bad for different files to have different values.

[2] https://bugs.webkit.org/show_bug.cgi?id=189541

I agree with the other comments that the template function should be defined in the header in the first place. It’s bad for different files to have different definitions of the same member function.

Sometimes it might be right to exclude files from unification, but in these two cases it seems like the unified build caught real problems with the code that should probably be fixed regardless.

[3] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70

On 02/09/2018 02:14, Simon Fraser wrote:
Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials. Here's an example: https://bugs.webkit.org/show_bug.cgi?id=189223. That patch added on new file in Source/WebCore/rendering, which required unrelated #include changes in at least:

rendering/RenderBlockFlow.cpp:
rendering/RenderFrame.cpp:
rendering/RenderImage.cpp:

How can we solve this? Should we have an EWS that builds non-unified? Can we somehow have the style checker do #include enforcement?

Simon

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


--
Frédéric Wang - frederic-wang.fr


_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev