[webkit-dev] Unified source builds: A new rule for static variables

[Ryosuke Niwa]

On Tue, Aug 29, 2017 at 2:14 PM, Keith Miller <keith_miller at apple.com> wrote:
>
>
>> On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak <mjs at apple.com> wrote:
>>
>>
>>
>>> On Aug 29, 2017, at 11:31 AM, Darin Adler <darin at apple.com> wrote:
>>>
>>> Sent from my iPhone
>>>
>>>> On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_miller at apple.com> wrote:
>>>>
>>>> I doubt anyone is going to run such a script before they go to upload a patch to bugzilla.
>>>
>>> EWS was what I was hoping for; likely to be sufficient. But it could also be integrated into the development process as, say, check-webkit-style is.
>>
>> check-webkit-style is run by both EWS and webkit-patch upload, in addition to being hand-runnable, so that seems like a good place to put this new kind of check.
>
> I agree and my intention was to add the check to check-webkit-style.
>
>>
>>>
>>>> So developers will still hit the name collision issue randomly throughout development.
>>>
>>> Sure.
>>>
>>> But I don’t think that required extensive use of namespaces is the best way to greatly mitigate this. Mistakes will still happen. So I think we shouldn’t go too far in ruining readability of code for something that is not necessary to solve the problem.
>>>
>>> Recommending either namespaces or globally unique names and clarifying that file local scope doesn’t exist are both good.
>>>
>>> But again I think people already handle these problems fine in headers so we don’t need too tight a straitjacket, at least not out of the gate.
>>
>> I tend to agree with this. I think keeping names of static functions globally unique is reasonable, so long as we have an automated way to check. This seems better than namespaces. With namespaces, it's still possible to make a mistake, such as by having a using at global scope, so we'd need the style checker to enforce some kind of rule.
>>
>> If we were to use namespaces, then properly naming them seems better than the FILENAME macro.
>
> I’ll defer to your judgement on properly naming the namespaces over using the macro. Does anyone have strong opinions on what rules the names should follow? I think Darin suggested <Filename>Internal. I think I would prefer <Filename>Static as that’s very clear what the namespace represents. I think I have been convinced that, for the most part we shouldn’t need to have such a strict rule on naming collisions. There are probably places where it makes more sense to have the namespace and places where it doesn’t.

As Darin suggested, <Class>Internal is appropriate for files like
Element.cpp, Node.cpp, etc... which contains definitions of a class.
There's an existing convention in WebKit that implementation details
are suffixed by "Internal". It's probably more appropriate to use
<Filename>Static for files like Editing.cpp, which is a collection of
a bunch of global functions.

> Here’s my proposal for the style checker. It should require that there are no duplicate globally visible variables in a given directory. We don’t need to worry about different directories since those files can’t be included in the same bundle under my proposal. While, it could be inconvenient for new developers it should keep the code much cleaner. Additionally, we can have a bot that builds with full per directory includes to ensure that we don’t have any collisions. As an aside, we might also want a bot that builds without unified sources to ensure people don’t rely on headers that happen to be above them in the bundle.

Makes sense.

- R. Niwa