[prev in list] [next in list] [prev in thread] [next in thread]
List: webkit-dev
Subject: Re: [webkit-dev] Unified sources have broken our #include hygiene
From: Maciej Stachowiak <mjs () apple ! com>
Date: 2018-09-13 16:50:47
Message-ID: 1A1AD7D4-6C90-42B4-A505-1B35DC662B89 () apple ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> 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 \
> <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@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
[Attachment #5 (text/html)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote \
type="cite" class=""><div class="">On Sep 13, 2018, at 3:49 AM, Frédéric Wang \
<<a href="mailto:fwang@igalia.com" class="">fwang@igalia.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi,<br \
class=""><br class="">I've recently found two non-trivial issues with unified builds. \
They are<br class="">not missing #include but instead conflicts between C++ files:<br \
class=""><br class="">- Two C++ files including the same header but with a #define \
directive<br class="">set to different values [1].<br class="">- One C++ file \
including a header with implicit template instantiation<br class="">in an inline \
function before an explicit template specialization in a<br class="">second C++ file. \
[2]<br class=""><br class="">I guess there could be more examples like this (e.g. \
functions with<br class="">conflicting names in two C++ files...). Should we allow to \
prevent some<br class="">files to be included in unified build? This is possible in \
Mozilla's<br class="">build system for example [3].<br class=""><br class="">Or \
should we always try to re-write the code to fix the conflicts<br class="">between \
files? This might go against our usual practice: For example the<br class="">current \
solution I have for [2] is to move the function from the header<br class="">to its \
implementation C++ file, losing potential performance benefit of<br \
class="">inlining...<br class=""><br class="">[1] <a \
href="https://bugs.webkit.org/show_bug.cgi?id=189579" \
class="">https://bugs.webkit.org/show_bug.cgi?id=189579</a><br \
class=""></div></div></blockquote><div><br class=""></div><div>^ 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.</div><br class=""><blockquote type="cite" class=""><div \
class=""><div class="">[2] <a href="https://bugs.webkit.org/show_bug.cgi?id=189541" \
class="">https://bugs.webkit.org/show_bug.cgi?id=189541</a><br \
class=""></div></div></blockquote><div><br class=""></div><div>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.</div><div><br class=""></div><div>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.</div><br \
class=""><blockquote type="cite" class=""><div class=""><div class="">[3] <a \
href="https://dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70" \
class="">https://dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70</a><br \
class=""><br class="">On 02/09/2018 02:14, Simon Fraser wrote:<br \
class=""><blockquote type="cite" class="">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.<br class=""><br \
class="">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: \
<a href="https://bugs.webkit.org/show_bug.cgi?id=189223" \
class="">https://bugs.webkit.org/show_bug.cgi?id=189223</a>. That patch added on new \
file in Source/WebCore/rendering, which required unrelated #include changes in at \
least:<br class=""><br class="">rendering/RenderBlockFlow.cpp:<br \
class="">rendering/RenderFrame.cpp:<br class="">rendering/RenderImage.cpp:<br \
class=""><br class="">How can we solve this? Should we have an EWS that builds \
non-unified? Can we somehow have the style checker do #include enforcement?<br \
class=""><br class="">Simon<br class=""><br \
class="">_______________________________________________<br class="">webkit-dev \
mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" \
class="">webkit-dev@lists.webkit.org</a><br \
class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br \
class=""></blockquote><br class=""><br class="">-- <br class="">Frédéric Wang - <a \
href="http://frederic-wang.fr" class="">frederic-wang.fr</a><br class=""><br \
class=""><br class="">_______________________________________________<br \
class="">webkit-dev mailing list<br class=""><a \
href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br \
class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br \
class=""></div></div></blockquote></div><br class=""></body></html>
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic