[webkit-dev] Unified sources have broken our #include hygiene

[Maciej Stachowiak]

> On Sep 13, 2018, at 3:49 AM, Frédéric Wang <fwang at 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 <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 <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 at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> 
> -- 
> Frédéric Wang - frederic-wang.fr
> 
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20180913/900457d0/attachment.html>