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

[Keith Miller]

In addition to what Darin said, I think using a consistent name makes the most 
sense. If “Internal” is what we would use for some of the places we 
should use it everywhere.

Cheers,
Keith

> On Aug 29, 2017, at 5:29 PM, Darin Adler  wrote:
> 
>> On Aug 29, 2017, at 4:32 PM, Ryosuke Niwa > > wrote:
>> 
>> It's probably more appropriate to use Static for files like 
>> Editing.cpp, which is a collection of a bunch of global functions.
> 
> The keyword “static” is being used to ask for what C++ calls “internal 
> linkage”  >.
> 
> So we could even consider “internal” in those cases, unless we can come up 
> with even better words.
> 
> — Darin

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

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

[Ryosuke Niwa]

On Tue, Aug 29, 2017 at 2:14 PM, Keith Miller  wrote:
>
>
>> On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak  wrote:
>>
>>
>>
>>> On Aug 29, 2017, at 11:31 AM, Darin Adler  wrote:
>>>
>>> Sent from my iPhone
>>>
 On Aug 29, 2017, at 11:22 AM, Keith Miller  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 Internal. I think I would prefer 
> 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, 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
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
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Keith Miller]

> On Aug 29, 2017, at 11:37 AM, Maciej Stachowiak  wrote:
> 
> 
> 
>> On Aug 29, 2017, at 11:31 AM, Darin Adler  wrote:
>> 
>> Sent from my iPhone
>> 
>>> On Aug 29, 2017, at 11:22 AM, Keith Miller  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 Internal. I think I would prefer 
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. 

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.

Does this seem reasonable?

> 
> Regards,
> Maciej

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

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

[Michael Catanzaro]

On Tue, Aug 29, 2017 at 1:37 PM, Maciej Stachowiak  
wrote:
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.


Agreed.

I posted a somewhat different proposal in the other thread, suggesting 
that we manually decide which files to unify together so that we only 
need to keep names of static functions unique to a particular unified 
source bundle. That would be easier than keeping the names 
globally-unique, but would complicate the build system. Either way 
would be fine I think: I'm really just hoping to avoid the need to 
adopt this namespace FILENAME rule. :)


Michael

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

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

[Maciej Stachowiak]

> On Aug 29, 2017, at 11:31 AM, Darin Adler  wrote:
> 
> Sent from my iPhone
> 
>> On Aug 29, 2017, at 11:22 AM, Keith Miller  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.

> 
>> 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.

Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Darin Adler]

Sent from my iPhone

> On Aug 29, 2017, at 11:22 AM, Keith Miller  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.

> 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.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Keith Miller]

> On Aug 29, 2017, at 10:15 AM, Darin Adler  wrote:
> 
> If we decide that we can’t support file scope identifiers then we should 
> figure out the most practical way to do it. Of course this affects constants 
> and variables, too, not just functions.
> 
> I think this special FILENAME namespace isn’t all that helpful or needed. If 
> a file contains a class like, say, Element, then we can name the namespace 
> ElementInternals or whatever else seems logical. Calling it FILENAME instead 
> doesn’t make things more readable nor more foolproof and is also likely to 
> confuse development tools unnecessarily. Note that we can use “using 
> namespace” inside functions if needed, but not at the file level.

I’m not particularly opposed to using a different namespace name. 
Unfortunately, that still doesn’t help with the ElementInternal:: invocation 
problem since you can’t forward ‘using namespace ElementInternal’ inside the 
class declaration of Element.

> 
> I think the harder part is how to enforce this rule if the theory is that we 
> can’t avoid conflicts with ad hoc naming. Using namespaces isn’t fool proof 
> unless there is something that checks for accidental use of the global scope.

I agree that we will need have some tool that prevents things from being placed 
into the global scope. My plan was to do that after getting the build working, 
with the belief that the issue would be relatively contained for the brief 
period between when we get unified builds working and the linter is place. This 
gets the most value to developers as soon as possible, which seems very 
valuable.

> 
> I don’t see that universal use of namespaces is required to avoid conflicts. 
> We manage to keep things unique across the whole project in headers using a 
> combination of namespaces and naming and I don’t see why it would require 
> such a firm rule inside source files as long as we clarify the uniqueness 
> requirement.

To be clear, I don’t think it’s required. Many other projects use unified 
source builds without using namespaces to solve the problem. My primary 
reasoning for using namespaces is a combination of avoiding the mental cost of 
thinking about the name collisions and just applying a simple rule. I’d also 
like to make this easier to understand for WebKit noobies, which is why I 
thought FILENAME would be simpler to wrap their heads around.

> 
> So I think we should not do the FILENAME thing and we should maybe not even 
> emphasize mandatory use of namespaces to avoid conflicts. Instead I suggest 
> we focus on making sure we tools that help us detect conflicts before code is 
> checked in even before we make the transition to this new way of building.

Without the namespace rule, any tool that checks for global namespace 
collisions won’t be all that useful for regular development. I doubt anyone is 
going to run such a script before they go to upload a patch to bugzilla. Even 
if they did it’s not clear that it would help them any more than just trying to 
compile and fixing the issue after they see the build error. So developers will 
still hit the name collision issue randomly throughout development.

> 
> — Darin

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

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

[Darin Adler]

If we decide that we can’t support file scope identifiers then we should figure 
out the most practical way to do it. Of course this affects constants and 
variables, too, not just functions.

I think this special FILENAME namespace isn’t all that helpful or needed. If a 
file contains a class like, say, Element, then we can name the namespace 
ElementInternals or whatever else seems logical. Calling it FILENAME instead 
doesn’t make things more readable nor more foolproof and is also likely to 
confuse development tools unnecessarily. Note that we can use “using namespace” 
inside functions if needed, but not at the file level.

I think the harder part is how to enforce this rule if the theory is that we 
can’t avoid conflicts with ad hoc naming. Using namespaces isn’t fool proof 
unless there is something that checks for accidental use of the global scope.

I don’t see that universal use of namespaces is required to avoid conflicts. We 
manage to keep things unique across the whole project in headers using a 
combination of namespaces and naming and I don’t see why it would require such 
a firm rule inside source files as long as we clarify the uniqueness 
requirement.

So I think we should not do the FILENAME thing and we should maybe not even 
emphasize mandatory use of namespaces to avoid conflicts. Instead I suggest we 
focus on making sure we tools that help us detect conflicts before code is 
checked in even before we make the transition to this new way of building.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

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

[Keith Miller]

> On Aug 29, 2017, at 9:04 AM, Chris Dumez  wrote:
> 
> I indeed think this will require “using” statements or explicit namespace at 
> call sites.

Yeah, this is basically what’s required. Unfortunately, if you ‘using 
namespace’ in a namespace all subsequent copies of that namespace will also see 
the ‘using’. e.g.

namespace Foo {
namespace Bar {
int myVar { 0 };
}
using namespace Bar;
}

namespace Foo {
namespace Baz {
int myVar { 0 };
}
using namespace Baz;

int myFunction() { return myVar; }
}

Will fail since myVar could be Baz::myVar or Bar::myVar. Using ‘using 
namespace’ inside a function or class body should be fine however.

> 
> I don’t think anonymous namespaces are suitable for resolving naming 
> conflicts due to unity builds since the functions and up in the same 
> compilation unit.

That’s right.

> 
> --
>  Chris Dumez
> 
> 
> 
> 
>> On Aug 29, 2017, at 9:00 AM, Darin Adler > > wrote:
>> 
>> How does this work? Without a “using” how does it know to search this 
>> namespace? Is this superior to using anonymous namespaces instead of 
>> “static”?
>> 
>> — Darin
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 

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

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

[Chris Dumez]

I indeed think this will require “using” statements or explicit namespace at 
call sites.

I don’t think anonymous namespaces are suitable for resolving naming conflicts 
due to unity builds since the functions and up in the same compilation unit.

--
 Chris Dumez




> On Aug 29, 2017, at 9:00 AM, Darin Adler  wrote:
> 
> How does this work? Without a “using” how does it know to search this 
> namespace? Is this superior to using anonymous namespaces instead of “static”?
> 
> — Darin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

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

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

[Darin Adler]

How does this work? Without a “using” how does it know to search this 
namespace? Is this superior to using anonymous namespaces instead of “static”?

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev