On Tue, Aug 29, 2017 at 2:14 PM, Keith Miller <keith_mil...@apple.com> wrote:
>> On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak <m...@apple.com> wrote:
>>> On Aug 29, 2017, at 11:31 AM, Darin Adler <da...@apple.com> wrote:
>>> Sent from my iPhone
>>>> On Aug 29, 2017, at 11:22 AM, Keith Miller <keith_mil...@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
webkit-dev mailing list

Reply via email to