Re: [webkit-dev] Proposal to change nested namespace indentation rule to match the existing code

2021-10-22 Thread Darin Adler via webkit-dev
> On Oct 22, 2021, at 12:33 AM, Kimmo Kinnunen via webkit-dev 
>  wrote:
> 
> 1) Match what seems to already be de-facto used in code.

Yes, does seem to be what we’re already doing. I like it. The rest of your 
arguments also seem good.

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


Re: [webkit-dev] Moving from WTF::Optional to std::optional

2021-06-02 Thread Darin Adler via webkit-dev
Thanks for pointing that out, Chris.

This advice goes beyond std::optional, by the way. For anything that we move 
from, there are two operations at are intended to be safe, from a C++ language 
and library design point of view: destroying the object and overwriting it by 
assigning a new value. If our code relies on doing anything else after the 
object is moved from, like examining the value after the old value is moved 
out, please use std::exchange to set the new value while moving the old value 
out. This even applies to large-seeming objects like HashMap, which I will note 
is not large: in release builds a HashMap is implemented as a single pointer to 
a structure on the heap and a new empty HashMap is a null pointer.

— Darin

Sent from my iPad

> On Jun 1, 2021, at 10:01 PM, Chris Dumez  wrote:
> 
> Hi,
> 
> Another thing Darin didn’t mention but I think people should be careful about:
> The move constructor for std::optional does not clear the is-set flag (while 
> the one for WTF::Optional did).
> 
> As a result, you will be having a very bad time if you do a use-after-move of 
> a std::optional. Please make sure to use std::exchange() instead of WTFMove() 
> if you want to leave to std::optional in a clean state for reuse later.
> 
> Chris Dumez
> 
>>> On Jun 1, 2021, at 8:54 PM, Darin Adler via webkit-dev 
>>>  wrote:
>>> 
>> Hi folks.
>> 
>> We’re getting rid of the WTF::Optional class template, because, hooray, 
>> std::optional is supported quite well by all the C++17 compilers we use, and 
>> we don’t have to keep using our own special version. Generally we don’t want 
>> to reimplement the C++ standard library when there is not a significant 
>> benefit, and this is one of those times.
>> 
>> Here are a few considerations:
>> 
>> 1) Since https://commits.webkit.org/238290@main, if you use Optional<> by 
>> mistake instead of std::optional<>, your code won’t compile. (Unless you are 
>> writing code for ANGLE, which has its own separate Optional<>.)
>> 
>> 2) If you want to use std::optional, include the C++ standard header, 
>> , or something that includes it. In a lot of cases, adding an 
>> include will not be required since it’s included by widely-used headers like 
>> WTFString.h and Vector.h, so if you include one of those are covered. 
>> Another way to think about this is that if your base class already uses 
>> std::optional, then you don’t need to include it.
>> 
>> 3) Once the patch in https://bugs.webkit.org/show_bug.cgi?id=226437 lands, 
>> includes of  won’t forward declare optional, and includes of 
>>  won’t do anything at all.
>> 
>> — 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


[webkit-dev] Moving from WTF::Optional to std::optional

2021-06-01 Thread Darin Adler via webkit-dev
Hi folks.

We’re getting rid of the WTF::Optional class template, because, hooray, 
std::optional is supported quite well by all the C++17 compilers we use, and we 
don’t have to keep using our own special version. Generally we don’t want to 
reimplement the C++ standard library when there is not a significant benefit, 
and this is one of those times.

Here are a few considerations:

1) Since https://commits.webkit.org/238290@main, if you use Optional<> by 
mistake instead of std::optional<>, your code won’t compile. (Unless you are 
writing code for ANGLE, which has its own separate Optional<>.)

2) If you want to use std::optional, include the C++ standard header, 
, or something that includes it. In a lot of cases, adding an include 
will not be required since it’s included by widely-used headers like 
WTFString.h and Vector.h, so if you include one of those are covered. Another 
way to think about this is that if your base class already uses std::optional, 
then you don’t need to include it.

3) Once the patch in https://bugs.webkit.org/show_bug.cgi?id=226437 lands, 
includes of  won’t forward declare optional, and includes of 
 won’t do anything at all.

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


Re: [webkit-dev] Let's use -Werror on EWS?

2021-05-24 Thread Darin Adler via webkit-dev
Sorry, I should clarify.

Apple’s ports of WebKit already use -Werror and always have, everywhere, not 
just on EWS. Since day one.

I do not know why we do not already use -Werror on GTK and WPE and I support 
using it there after fixing all the warnings.

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


Re: [webkit-dev] Let's use -Werror on EWS?

2021-05-24 Thread Darin Adler via webkit-dev
I’m a big fan of -Werror. Back when WebKit started, it was controversial to use 
it at Apple.

But we can’t just turn on -Werror until after we fix all the warnings! Who will 
do that project.

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


Re: [webkit-dev] New EWS Non-Unified builder

2021-04-30 Thread Darin Adler via webkit-dev
OK. I acknowledge my view on this is different from the others commenting here, 
perhaps because of different ideas about what is hard and requires expertise. I 
won’t stand in the way of changes you want to make. You know my view now.

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


Re: [webkit-dev] New EWS Non-Unified builder

2021-04-29 Thread Darin Adler via webkit-dev
> On Apr 29, 2021, at 9:06 PM, Tim Horton via webkit-dev 
>  wrote:
> 
> it is definitely highly annoying

It’s possible that where my thinking differs from others on this is that I 
don’t think shifting annoying work from one set of commits (the ones adding a 
new file) to a different set (the ones unknowingly adding need for a new 
include for non-unified builds) is a significant improvement. Adding more EWS 
bubbles has a cost.

Keep in mind that I am just as passionate as the next person about getting 
includes “right”. I’m just not sure that this would be a step in the right 
direction.

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


Re: [webkit-dev] New EWS Non-Unified builder

2021-04-29 Thread Darin Adler via webkit-dev
> On Apr 29, 2021, at 9:01 PM, Tim Horton  wrote:
> 
> This is a huge problem for people on the Apple platforms using the Xcode 
> projects. Nearly every time someone adds a file they have to add random 
> headers to unrelated code.

OK, sorry, I must be out of touch with the rest of you about how difficult this 
is. I’ve done that many times, and it was always easy for me.

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


Re: [webkit-dev] New EWS Non-Unified builder

2021-04-29 Thread Darin Adler via webkit-dev
> On Apr 29, 2021, at 12:04 PM, Ross Kirsling via webkit-dev 
>  wrote:
> 
> Yeah, I think it's important to clarify that nobody is "using 
> non-Unified-Source building for their development", at least to my knowledge. 
> Being broken by the shifting sands of unified sources is an everybody problem 
> (or at the very least an "everybody that builds via CMake problem", which is 
> ultimately an everybody problem).

How is this a bigger problem in CMake than for people on the Apple platforms 
who are using Xcode projects? Can we create greater parity between the two?

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


Re: [webkit-dev] New EWS Non-Unified builder

2021-04-29 Thread Darin Adler via webkit-dev
Given the issue is that there are people that are using non-Unified-Source 
building for their development, the best fix is to add post-commit and EWS 
builders for one of those platforms. I do not support the idea of adding an 
additional builder just to “keep non-Unified-Source building” if no 
actively-supported platform is not choosing that build style.

Specifically, if Sony people are most affected by this, I suggest we find a way 
to add Sony PlayStation post-commit and EWS builders.

I am not convinced that we should add some kind of abstract “correct 
compilation” that is a separate builder.

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


Re: [webkit-dev] Mailman web interface missing

2021-04-26 Thread Darin Adler via webkit-dev
> On Apr 26, 2021, at 1:03 PM, Adrian Perez de Castro via webkit-dev 
>  wrote:
> 
> - https://lists.webkit.org/listinfo/webkit-wpe/ results in “Not Found”
> - https://lists.webkit.org/admindb/webkit-wpe/ results in “403 Forbidden”

I took a look and it seems that the URLs have simply changed, perhaps as a 
result of an update:

https://lists.webkit.org/mailman/listinfo/webkit-wpe
https://lists.webkit.org/mailman/admin/webkit-wpe 


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


Re: [webkit-dev] GitHub Account and Commit Attribution

2020-12-17 Thread Darin Adler via webkit-dev
> On Dec 17, 2020, at 8:47 AM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> Something we’ve just learned about commit attribution and GitHub is that 
> adding an email to your GitHub account may not attribute commits that were 
> pushed to a repository before you added the email. There were a few issues 
> with history as it stands now, and I will be pushing up the final version of 
> history in the next few days.
> 
> What this means for you now is that it is time to set-up your GitHub account 
> if you have not already and add any emails you used to commit to WebKit to 
> your account (GitHub allows multiple emails to be associated with a single 
> account).

Do GitHub’s privacy settings matter? My GitHub account now has da...@apple.com 
associated with it, but I have "Keep my email addresses private” checked in 
GitHub settings. Does that matter?

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


Re: [webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

2020-12-09 Thread Darin Adler via webkit-dev
> On Dec 9, 2020, at 1:02 PM, Geoff Garen via webkit-dev 
>  wrote:
> 
>> - Make FontCache thread-safe, but do it via introducing a completely
>> separate thread-safe AtomString type and leave the current one as it is
>> (I don't have a good grasp of how difficult this would actually be)
> 
> I had to chuckle at this point because the obvious name for this new 
> thread-safe AtomString class would be AtomicString, the prior name of 
> AtomString.

If we make a separate thread-safe code path, I’m not sure we need to create a 
variant of AtomString at all.

AtomString optimizes string comparisons (by paying up front every time you 
construct one with a hash table lookup) and memory use (by sharing the same 
memory for all equal strings), but there’s no reason we can’t compare two 
strings without using AtomString. I’m doubting that once we figure out what 
we’re trying to do that we’ll need AtomString.

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


Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

2020-10-20 Thread Darin Adler
> On Oct 20, 2020, at 4:08 PM, Sam Weinig  wrote:
> 
> For the ones conditionalizing adding to ADDITIONAL_BINDING_IDLS, those IDLs 
> should just have the appropriate Conditional=* extended attribute added, then 
> they can be included unconditionally in the makefile.
> 
> For the ones conditionalizing adding go USER_AGENT_STYLE_SHEETS, again, there 
> doesn’t seem a real reason to keep that. The sheets are all preprocessed 
> anyway, so we can just move those #ifdefs into the files.
> 
> I filed https://bugs.webkit.org/show_bug.cgi?id=218001 
>  for this and will get that 
> done regardless.

Some of these also make it possible to build without WebKitAdditions. We may 
need some different approach to that.

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


Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

2020-10-20 Thread Darin Adler
> On Oct 20, 2020, at 3:09 PM, Sam Weinig  wrote:
> 
> For the Platform.h issue, I think we could probably engineer things in the 
> short term to have a script phase that produces a Defines.txt that the other 
> script phases and build rules depend on.

We definitely can, and it’s simpler than how we do it now. I have a patch that 
does this for the makefile partly done and set aside. My approach was to revise 
each script, one at a time, to take a file with the defines rather than 
requiring they be passed on the command line. It doesn’t work for the parts of 
the makefile itself that depend directly on checking the defines (grep for 
findstring.*FEATURE_AND_PLATFORM_DEFINES to see those).

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


Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

2020-10-18 Thread Darin Adler
It would be wonderful to drastically cut down the repeated mentions of the same 
filenames. People not on Xcode platforms probably focus primarily on Xcode 
projects for this frustration. But I find it ridiculous how many places I have 
to add things. For example, to add AbstractRange I had to add filenames to:

Shared

- Sources.txt: Two source files and one IDL-generated file.

CMake:

- CMakeLists.txt: IDL file.
- Headers.cmake: One source file and one IDL-generated file.

Xcode:

- DerivedSources.make: IDL file
- WebCore.xcodeproj/project.pbxproj: IDL file, two source files, and Two 
IDL-generated files.

And also, updating these checked-in generated files (that we’d need regardless 
of build system, due to Apple build requirements):

- DerivedSources-input.xcfilelist
- DerivedSources-output.xcfilelist

It seems like we should have a goal that you should not have to list 
IDL-generated file names. That will be hard to accomplish in Xcode, though. 
Maybe the reason we haven’t been so ambitious about getting this right for 
CMake is that it’s so unreachable a goal for Xcode, given that we want the 
files to show up in a clean way in the IDE?

And it would be good to find a way to share the lists of IDL files like we do 
the list of source files. That seems relevant to this discussion of changing 
DerivedSources.make.

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


Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

2020-10-18 Thread Darin Adler
Oops, I was in the middle of writing this and hit send by mistake
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

2020-10-18 Thread Darin Adler
Please note that this is a Cocoa-only, Xcode-only conversation.

As Sam pointed out, there’s a whole different “will you please switch to CMake” 
conversation. That’s not an easy switch to make for programmers working on the 
Apple platforms, but we might do it at some point if we can work out the issues 
Sam mentioned.

> On Oct 17, 2020, at 10:00 AM, Sam Weinig  wrote:
> 
> While I can’t recall the initial reasons for using Makefiles for derived 
> sources, over the years there have been a number of advantages to it that I 
> do know of. One clear advantage, that is no longer applicable, was code 
> sharing, as earlier in the project, at least the Apple Windows port did 
> utilize these Makefiles.

Pretty sure I was the one who did this. And I do recall the initial reasons. 
There were only two.

1) Code sharing

I believed at the time that the rules for building derived sources were tricky 
and hard to get right, and proposed that we would use the 
lowest-common-denominator build system, make, across all platforms. This 
strategy didn’t work because right from the beginning people working with 
various build systems used copies of the derived sources rules that I wrote 
rather than invoking make, and never tried sharing code.

I think this was a missed opportunity to share complicated code, but it 
happened a very long time ago, and I made my peace with it back then.

2) Xcode build system deficiencies

At that time, Xcode’s build system had too many problems for these kinds of 
complex rules. Many years later, that may no longer be true.

Since we’re not even trying to do the code sharing, I think it’s fine to stop 
using makefiles now.

If we’re deeply committed to switching to CMake at some point in future, one 
way to take a step in that direction would be to return to the original code 
sharing goal, and use CMake for derived sources, so we have code sharing. I 
don’t know how modular CMake is and how practical it is to use it for part, but 
not all, of the build process with Xcode. People using CMake for everything 
might be frustrated with the constraints this could place on how CMake is used.

I think an important third goal that I did not have at the start, but have now, 
is the one Ryosuke mentioned:

3) Minimize the number of places files are listed, so it’s easy to add, remove, 
and rename files of various types.

It would be wonderful to drastically cut down the repeated mentions of the same 
filenames. People not on Xcode platforms probably focus primarily on Xcode 
projects for this frustration. But I find it ridiculous how many places I have 
to add things. For example, to add AbstractRange I had to add filenames to:

Shared:

Sources.txt

CMake:

CMakeLists.txt
Headers.cmake

Xcode:

WebCore.xcodeproj/project.pbxproj

And also, the checked-in generated files (that we’d need regardless of build 
systems, due to Apple build requirements):

DerivedSources-input.xcfilelist
DerivedSources-output.xcfilelist

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


Re: [webkit-dev] Trailing space changes in "expected.txt" files

2020-09-26 Thread Darin Adler
> On Sep 26, 2020, at 8:13 PM, Darin Adler  wrote:
> 
> by rebasing and either regenerating the expected.txt files hand editing.

… resolve the conflicts by rebasing and either regenerating the expected.txt 
files with run-webkit-tests or by hand editing the expected.txt files.

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


[webkit-dev] Trailing space changes in "expected.txt" files

2020-09-26 Thread Darin Adler
Hi folks.

This afternoon, I changed dumpAsText so it strips trailing spaces.

https://trac.webkit.org/changeset/267640

As part of that change, run-webkit-tests temporarily will ignore trailing 
spaces in expected.txt files, so they can be checked in with, or without, the 
spaces. Over the next few days, I’ll be checking in new versions of 
expected.txt files with the spaces at the ends of lines stripped. Once we have 
all the spaces stripped, then run-webkit-tests will go back to being strict 
about this again.

This affects you if you have a patch that changes an expected.txt file; the 
space changes might lead to patch conflicts. If you do encounter conflicts, you 
can resolve them by rebasing and either regenerating the expected.txt files 
hand editing. Any newly-regenerated files won’t have any trailing spaces, and 
it’s best if newly checked in files do not have trailing spaces.

If you run into any trouble with this, please get in touch.

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


Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

2020-09-23 Thread Darin Adler
> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
> 
> There are quite a few cases where data members are references but then those 
> can also be replaced by a simple member function which retrieves the value of 
> the smart pointer member variable and returns a reference.

I think this should be an explicit recommendation in the project of refactoring 
to follow these rules.

> For now, a trivial function is defined as a member function defined in the 
> class declaration whose definition simply returns a member variable (the 
> result of get() or a copy if the member variable is a smart pointer).

That seems like a rule that’s too narrow. I would not want a function to become 
non-trivial just because I moved it from being inline within the class 
definition to an inline below the class definition in the same header.

This rule worries me a lot right now; it seems like it could result in an 
explosion of local variable copies of arguments.

> We probably also need to figure out a way to exempt all lambda functions that 
> never get stored anywhere. We have a bunch of helper functions like WTF::map 
> which just calls lambdas on each item while iterating over an array, etc... 
> and there is no need to create a separate Ref / RefPtr in those cases since 
> lambdas are never stored and re-used later.

Does seem important. I am pretty sure I have seen this concept in other 
languages. We often try to use const Function& for one type of lambda argument 
and Function&& for the other type, but that’s far from complete.

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


Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

2020-09-23 Thread Darin Adler
> On Sep 23, 2020, at 10:32 AM, Darin Adler  wrote:
> 
>> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa > <mailto:rn...@webkit.org>> wrote:
>> Every data member to a ref counted object must use either Ref, RefPtr, or 
>> WeakPtr. webkit.NoUncountedMemberChecker 
>> <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id17>My
>>  only worry here is performance. Do we know yet if we can afford it?
> 
> The worst case here is Ref, which is much worse than a reference since we end 
> up having to use -> instead of . everywhere and you can’t see that there is 
> no null involved.

I don’t mean performance worst case. I meant “outside of performance concerns, 
the worst downgrade of our programming idioms is Ref”.
>> Every ref counted base class, if it has a derived class, must define a 
>> virtual destructor. webkit.RefCntblBaseVirtualDtor 
>> <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id16>The
>>  style system has an optimization that intentionally violates this for 
>> performance reasons (StyleRuleBase).

So are we saying we must not do that optimization, or will we have a way to 
suppress the warning for this?

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


Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

2020-09-23 Thread Darin Adler
> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa  wrote:
> Every data member to a ref counted object must use either Ref, RefPtr, or 
> WeakPtr. webkit.NoUncountedMemberChecker 
> My
>  only worry here is performance. Do we know yet if we can afford it?

The worst case here is Ref, which is much worse than a reference since we end 
up having to use -> instead of . everywhere and you can’t see that there is no 
null involved.
> Every ref counted base class, if it has a derived class, must define a 
> virtual destructor. webkit.RefCntblBaseVirtualDtor 
> The
>  style system has an optimization that intentionally violates this for 
> performance reasons (StyleRuleBase).
> Every ref counted object passed to a non-trivial function as an argument 
> (including "this" pointer) should be stored as a Ref or RefPtr in the 
> caller’s local scope unless it's an argument to the caller itself by 
> transitive property [1]. alpha.webkit.UncountedCallArgsChecker 
> What
>  is a non-trivial function?
> Every ref counted object must be captured using Ref or RefPtr for lambda. 
> webkit.UncountedLambdaCapturesChecker 
> Ref,
>  RefPtr, or WeakPtr, right?

Same concern about Ref vs references.
> Local variables - we’re still working on this 
> (https://reviews.llvm.org/D83259 )
I am looking forward to learning more about the proposal here.

Same concern about Ref vs. references.

I really want to see before/after for some non-trivial source files with 
significant problems; where will this drive the most change and what will 
things look like after?

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


[webkit-dev] Could use some help from people who care about the WinCairo port

2020-09-12 Thread Darin Adler
The patch for this bug

https://bugs.webkit.org/show_bug.cgi?id=216428 


is failing to build on WinCairo because of some kind of precompiled header 
problem. I could use some help figuring out what I did wrong.

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


Re: [webkit-dev] PSA: Bit fields won't be packed on Windows if you mix types

2020-09-04 Thread Darin Adler
> On Sep 3, 2020, at 11:14 PM, Fujii Hironori  wrote:
> 
> BTW, I don't like to idea adding a new rule, but keeping old style code. It 
> introduces divergence between the guideline and actual code. Do we really 
> need a new rule that one doesn’t necessary have to follow?

I understand that you don’t like this.

But all of our style rules are like this. We don’t apply them across our entire 
project at the moment we institute them. They guide the future of WebKit. So 
this is not a valid argument against adding a new rule.

You can help us apply a given rule to existing code if you feel strongly in a 
particular case, and in some cases it might be practical to quickly apply it 
everywhere relevant. Maybe in this case it is.

But that’s not the basis for deciding whether to make it a rule.

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


Re: [webkit-dev] Feedback on Blink's text fragment directive proposal

2020-08-31 Thread Darin Adler
> On Aug 31, 2020, at 9:33 AM, David Bokan  wrote:
> 
> We've made lots of improvements to the spec, notably around issues raised in 
> Mozilla's standards-position 
> .

Do you think those improvements address Maciej’s concerns from last December? 
Since you didn’t say that explicitly I was wondering what your take on that was.

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


Re: [webkit-dev] Plugin process

2020-07-20 Thread Darin Adler
> On Jul 20, 2020, at 7:35 AM, Michael Catanzaro  wrote:
> 
> Then we can make the plugin process specific to PLATFORM(COCOA)

Side note: It will be specific to PLATFORM(MAC).

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


Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-16 Thread Darin Adler
> On Jul 16, 2020, at 3:33 PM, Adrian Perez de Castro  wrote:
> 
> enabling/disabling features changes the set of source files to
> build, which results in different source files being #included from each
> unifier source compilation unit.

Let’s stop doing it that way. Just compile the files and have #if inside the 
file.

I removed all conditional source files from the Xcode build system. Let's do 
the same for the CMake build system.

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


Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-16 Thread Darin Adler
> On Jul 16, 2020, at 2:28 PM, Adrian Perez de Castro  wrote:
> 
> It is not feasible to test unified builds for all the possible combinations of
> target architectures and set of features enabled at build time (there is a
> combinatorial explosion of configurations). Keeping non-unified builds in a
> working state guarantees that regular unified builds will keep working no
> matter what the build configuration is.

But it doesn’t.

Non-unified builds don’t make it possible to have arbitrary sets of features 
enabled at build time. We break that all the time and it’s only fixed when 
someone tests that arbitrary set of features. Headers are only a small part of 
that issue.

I’d go as far as saying it’s not a project goal to always build with arbitrary 
sets of features enabled.

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


Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-16 Thread Darin Adler
> On Jul 16, 2020, at 12:54 PM, Kirsling, Ross  wrote:
> 
> Non-unified build fixes are done to support *all* platforms, because sudden 
> unification shifts can happen anywhere.

I’m not sure that benefit is worth the additional code churn. I understand that 
it’s frustrating to run into a problem when unification shifts, say when you 
are adding a source file. I believe we have made changes to unification since 
the earliest version that reduce how often unification shifts.

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


Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-14 Thread Darin Adler
> On Jul 14, 2020, at 2:38 PM, Simon Fraser  wrote:
> 
> Could someone educate me about ? When should I use this 
> instead of individual wtf headers?

Forward.h is analogous to forward-declaring a class ('class IntPoint;' instead 
of ‘#include “IntPoint.h”'), but it works for many often-used classes and class 
templates in the WTF namespace, including class templates that would be 
difficult to correctly forward-declare due to their many arguments, such as 
WTF::Vector. And it includes “using WTF::String” and the like, as well, to 
import WTF namespace things into the global namespace.

We can use it any time we need a forward-declaration, not an entire definition, 
of one of the items. For example, to compile a header that just takes and 
returns String objects, we only need a forward declaration of String. The 
easiest way to correctly do that is to include . Including 
 pulls in a lot more. For the specific case of String, I think 
you might be able to go further and write this instead:

namespace WTF {
class String;
}
using WTF::String;

But I have never tried it, and there might be some problem with that.

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


Re: [webkit-dev] [webkit-changes] [264332] trunk/Source

2020-07-14 Thread Darin Adler
We need to be cautious about adding header includes to other headers. Adding 
includes to .cpp files is likely fine and not a deeply consequential decision, 
but adding to headers is something that can have a massive effect on build 
times over time.

> On Jul 13, 2020, at 10:44 PM, hironori.fu...@sony.com wrote:
>  <>Modified: trunk/Source/WebCore/platform/graphics/ColorSerialization.h 
> (264331 => 264332)
> 
> --- trunk/Source/WebCore/platform/graphics/ColorSerialization.h   
> 2020-07-14 05:17:20 UTC (rev 264331)
> +++ trunk/Source/WebCore/platform/graphics/ColorSerialization.h   
> 2020-07-14 05:44:25 UTC (rev 264332)
> @@ -25,6 +25,8 @@
>  
>  #pragma once
>  
> +#include 
This change is wrong. The header to include here is Forward.h, not WTFString.h. 
Not super-harmful since WTFString.h is already so widely included, but we need 
to be really cautious in headers.

> Modified: trunk/Source/WebCore/svg/SVGParserUtilities.h (264331 => 264332)
> 
> --- trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:17:20 UTC 
> (rev 264331)
> +++ trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:44:25 UTC 
> (rev 264332)
> @@ -24,6 +24,7 @@
>  #include "ParsingUtilities.h"
>  #include 
>  #include 
> +#include 
Same mistake.

I’d really like to come up with some other system for reviewing adding headers 
to *headers*.

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


Re: [webkit-dev] Platform.h vs. makefiles

2020-05-11 Thread Darin Adler
> On May 10, 2020, at 10:07 PM, Maciej Stachowiak  wrote:
> 
> I think the best way to configure WebKit is to use a separate data file, 
> neither a header nor a makefile or similar, that defines the options in a 
> single place with clear syntax. Then everything else is generated from that. 
> Such a system could cover runtime flags as well, which are even more 
> scattered around the project than compile-time flags.

Sounds OK. I worry a little about defining yet another language, but otherwise 
I do find this appealing. Perhaps some people would say that 
FeatureDefines.props from cmake could be that file?

Not sure literally a single data file is the best way to do this across 
multiple platforms/ports. I think PlatformEnableCocoa.h shows us that breaking 
this down can be helpful.

One file that has the master list of all the settings, and all the default 
values. Then other files that contain overlays for each port/configuration 
where they are different from the default.

My worry is that it could become complicated, like our TestExpectations files, 
which were once simple.

> Moving logic from build files to the header is probably a move in the right 
> direction, though of course it carries risk, particularly for less tested 
> configurations.

Yes, I’m not suggesting rushing to do it all at once in a mass change.

But for new things especially on Apple’s Cocoa platforms, I’d like to avoid 
FeatureDefines.xcconfig and see new things in the PlatformEnableCocoa.h header 
file instead. Unless the defines need to affect the build system and not just 
the C++ code, I think the header file is superior.

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


[webkit-dev] Platform.h vs. makefiles

2020-05-10 Thread Darin Adler
Hi folks.

The Platform.h configuration file family has been useful for WebKit for a long 
time. We created it to try to organize configuration options in WebKit so the 
would not be spread out through the whole project.

One way to look at these, particularly the ENABLE options, is as a set of 
configuration options that let each consumer of the WebKit source code create a 
unique flavor of WebKit with the particular features they want enabled turned 
on and others turned off. Another is to think of this as a mechanism for 
keeping decisions made by the WebKit contributors organized and easy to 
understand.

If these truly are WebKit consumer options, then it’s important they be set as 
part of the build process. The macros can be defined with a build and 
configuration system outside WebKit, and the Platform.h headers should 
interpret those values correctly.

On the other hand, if we are just trying to keep our decisions straight, then 
it would be best if the logic for what is on and what is off by in the header 
files, written with preprocessor macro logic, and not spread across multiple 
make files and scripts.

Thus I think the pattern on macOS, for example, of setting these in .xcconfig 
files doesn’t make a lot of sense. I think the .xcconfig files should compute 
the things that need to be determined about the build environment, but the 
configuration decisions should be in files like PlatformHaveCocoa.h, for 
example.

I think the guideline should be like this:

All code to compute configuration should be in the Platform.h files, not in 
makefiles, with only the following exceptions:

1) Options like ENABLE(XXX) that some ports wish to offer as options to people 
building WebKit can have overridden values in makefiles. But even these options 
should have sensible defaults in the Platform.h headers that express the 
current status of the port from the point of view of the port’s maintainers. 
Ideally we’d find a way to not repeat these default settings twice.

2) In some cases, the build machinery needs to contribute to things like 
feature detection. So on some platforms, things like HAVE(READLINE) would be 
set correctly by the build system.

Options intended to be set by the environment would carefully be wrapped in 
#ifndef.

But other options, which simply express relationships between configuration 
elements, are designed to be set by Platform.h and not overridden by the 
environment, and so they would not be wrapped in #ifndef. Many HAVE options and 
most USE options would fall into this category.

What do you all think?

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


Re: [webkit-dev] Introducing a minimum ICU version for WebKit

2020-04-09 Thread Darin Adler
Here’s my take:

The source code we check into the WebKit repository is there for the 
convenience of the people *contributing* to WebKit, and is need not be the sole 
input when building and packaging WebKit for distribution.

Including ICU sources in a GTK WebKit tarball would not necessarily require 
checking ICU source code into the WebKit repository.

Everything else being equal, adding more files to the WebKit repository does 
make things less convenient for contributors. I would like us to do what we can 
to cut down the files, including things like unused PNG files in the 
LayoutTests directories, any source code that was needed at one time but is not 
needed in practice any more, including source code in the ThirdParty directory.

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


Re: [webkit-dev] Accidental binary bloating via C/C++ class/struct + Objective-C

2020-01-13 Thread Darin Adler
> On Jan 13, 2020, at 5:52 PM, Yusuke Suzuki  wrote:
> 
> We can purge type-encoding-string if we use Objective-C NS_DIRECT feature 
> (which makes Objective-C function as C function calling convention, removing 
> metadata).
> However, this does not work universally: with NS_DIRECT, Objective-C override 
> does not work. This means we need to be extra-careful when using it.

Yes, we need to be careful, but NS_DIRECT is likely to have more benefit than 
just binary shrinking, and it should do even more binary shrinking than hiding 
types from Objective-C. Use of NS_DIRECT will likely help performance, 
sometimes in a measurable way.

However, besides the risk of making something “non-overridable”, I think it’s 
only available in new versions of the clang compiler.

> So, as a simple, but effective work-around, in the above patch, we introduced 
> NakedRef / NakedPtr. This is basically raw pointer / raw reference to 
> T, with a wrapper class.
> This leverages the behavior of Objective-C compiler’s mechanism “one-level 
> deep type information collection”. Since NakedRef / NakedPtr introduces 
> one-level deep field,
> Objective-C compiler does not collect the type information of T if 
> NakedPtr is included in the fields / signatures, while the compiler 
> collects information when T* is used.

Very exciting. Does this cover all the cases we care about? Does come up for 
types that are not references or pointers? Maybe we can pass arguments by const 
reference? What about return values?

> ## Future work
> 
> We would like to avoid including such types accidentally in Objective-C. We 
> should introduce build-time hook script which detects such a thing.
> I uploaded the PoC script in https://bugs.webkit.org/show_bug.cgi?id=205968 
> , and I’m personally planning 
> to introduce such a hook into a part of build process.

Beautiful. Well worth doing.

Thanks!

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


Re: [webkit-dev] Introducing WTF::makeUnique and WTF::makeUniqueWithoutFastMallocCheck

2019-08-23 Thread Darin Adler
> On Aug 23, 2019, at 7:26 AM, Antti Koivisto  wrote:
> 
> Could WTF::makeUnique simply use FastMalloc by default? We could then remove 
> most of these messy annotations.
> 
> This would require replacing std::unique_ptr with a type that knows how to 
> free the objects correctly (bring back OwnPtr!) but that doesn't seem like a 
> big deal. It wouldn't play well with mixed use of OwnPtr and new/delete but 
> that should be avoided in any case.

I also suggested this, and you can see Yusuke’s response here 
.

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


Re: [webkit-dev] Moving to Python 3

2019-07-16 Thread Darin Adler
> On Jul 16, 2019, at 12:46 PM, Alexey Proskuryakov  wrote:
> 
> - They shouldn't make it excessively difficult to do WebKit engineering on 
> older versions of macOS.
> 
> "Excessively" is not clearly defined, but it seems obvious that there is a 
> tradeoff between tools work difficulty, and difficulty for engineers who go 
> back to older macOS versions (and implicitly, difficulty for people who set 
> up bots, QA engineers, and others involved).
> 
> I don't think that anyone ever suggested that it will be up to each engineer 
> to figure out the best way to install Python 3.

Lets keep in mind our strategy to keep development of WebKit on macOS easy. We 
want to preserve this. The steps (assuming git) are:

https://webkit.org/build-tools/
• download and install Xcode from Apple
% xcode-select --install

https://webkit.org/getting-the-code/
% git clone git://git.webkit.org/WebKit.git WebKit
% Tools/Scripts/webkit-patch setup-git-clone

https://webkit.org/building-webkit/
% build-webkit --debug

https://webkit.org/running-webkit/
% run-safari

We’d really like to keep it to a small number of steps. Having to download and 
install anything else would be a significant additional step.

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


Re: [webkit-dev] Incremental unified builds on macOS

2019-06-25 Thread Darin Adler
It’s unlikely the problem is due to unified builds, since those are just source 
files with includes in them.

But I have encountered problems like this before; sometimes they are things 
affecting lots of people and I’ve been able to fix them. When trying to 
understand causes of this kind of problem in the past, an Xcode build system 
feature that Dan Bernstein told me about was really useful. To use it you can 
type this command:

% defaults write com.apple.dt.Xcode ExplainWhyBuildCommandsAreRun -bool YES

Then rebuild and the log will contain additional information about each build 
step.

Good luck and please let us know what you find!

— Darin

PS: That should work for now since WebKit still uses the “legacy Xcode build 
system” but I hear it won’t work once we move to the modern build system.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Requesting feedback about EWS comments on Bugzilla bugs

2019-06-16 Thread Darin Adler
> On Jun 15, 2019, at 9:13 PM, Aakash Jain  wrote:
> 
> 1) Do not upload archive (for layout-test-results) on bugzilla, instead 
> upload it to another server, unzip it and post a link to the results.html.
> Pros:
> a) Engineers won't have to download the attachment, unzip it, look for 
> failures, and then delete it from their disk. They can simply click the url 
> to view the results. 
> b) This approach will also reduce 2 comments per failure to 1 comment. 
> Currently there are two comments per failure, one for failure details, second 
> for bugzilla attachment.

Great improvement to do this. The most confusing thing about build bot comments 
is all the “creation of attachments” extra text with things like “attachment 
number” and “patch".

However, it’s really nice that I can download a directory full of test results 
easily. I’d like to see the EWS website still have that feature.

> 4) When a patch becomes 'obsolete', tag the corresponding EWS comments as 
> 'obsolete', so that they will be hidden.

Incredibly valuable.

> 5) Do not comment on bugzilla bug at all

I think this makes sense. I don’t see a reason that test results need to be 
comments. I think the “red bubble” in EWS already calls someone’s attention to 
failures.

If we want to augment it, we should think of what we are aiming at. I do find 
it useful to see which tests are failing, and when I click on the red bubble I 
don’t see that information. I have to click once to see the “log of activities” 
then click on “results”, then see a confusing giant file with lots of other 
information. At the bottom of that file the one thing I want to know.

A better hierarchy is to put that “what new tests are failing” summary right t 
the top and let the logs be fallbacks, not the primary place to see the 
features.

> instead send email to the author of the patch.

Why? I don’t think this should send any emails at all, unless the person 
requested it.

> Pros: less noisy, also this will allow to include more detailed information 
> about the failure in email.

I think the more detailed information should be on the webpage, not in an email.

> Cons: reviewers would have to click status-bubbles to see the failures, 
> failure information is not immediately present in the comments.

I think we should start with this approach, eliminating the comments entirely.

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


Re: [webkit-dev] Changing the svn commit hook to allow tabs for tests.

2019-05-10 Thread Darin Adler
> On May 10, 2019, at 1:13 PM, Keith Miller  wrote:
> 
> I’m not sure I know what you mean by allow a whole-directory exception. Do 
> you mean a top level directory? Or some kind of parameter we pass to the hook 
> to ignore some directory for that run?

I meant that we could add something the pre-commit hook could see in Subversion 
that would create an exception for a whole directory, rather than something 
inside the hook itself. Perhaps a specially named file, or a Subversion 
attribute on a specially named file, or something more clever. If Subversion 
had attributes on directories, it could be that.

> you can’t commit with git-svn as it doesn’t support svn properties (or at 
> least I wasn’t able to figure it out)

Ah, that’s a big blocker if lots of people are using git-svn — I certainly use 
it.

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


Re: [webkit-dev] Changing the svn commit hook to allow tabs for tests.

2019-05-10 Thread Darin Adler
> On May 10, 2019, at 1:00 PM, Keith Miller  wrote:
> 
> I don’t know if this is possible but it would be great if some 
> sub-directories could be excluded from the no-tabs pre-commit hook.

Maybe we can rewrite the pre-commit hook to allow a whole-directory exception. 
Ideally I’d prefer not to hardcode directories.

> it’s pretty inconvenient to add the svn attribute that allows tabs every time 
> I update the tests

Does it really have to be inconvenient? Can we make script that does this and 
check it in so anyone can run it? Or build it into webkit-patch or whatever 
tool you already use?

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


Re: [webkit-dev] Spam and indexing

2019-05-02 Thread Darin Adler
Should we post instructions somewhere for people dealing with spam? I believe 
the instructions are:

1) Look up the email address of the account that posted the spam and disable it 
first, so spammers don’t get email about other steps. Do this by clicking on 
Administration, Users, finding the user and putting the word “Spam” into the 
disable text.

2) Move any spam bugs into the Spam component.

3) Mark any spam comments as Private and also add the tag “spam”.

But maybe there’s more to it than that. For example, can someone without 
administration privileges do the right thing? Should we make a small tool to 
make this easier to do correctly?

I like the idea of having instructions so this isn’t oral tradition.

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


Re: [webkit-dev] Code Style: Opinion on returning void

2019-02-21 Thread Darin Adler
I tried not to weigh in on this, but my view on the materials we should use to 
build the bike shed must be shared!

Generally it seems neat to be able to make the code slightly more tight and 
terse by merging the function call and the return into a single statement.

Other than not being accustomed to it, I have noticed three small things I 
don’t like about using “return” with a call to a void-returning function.

- You can’t use this idiom when you want to ignore the result of a function, 
only if the function actually returns void. Often functions are designed with 
ignorable and often ignored return values. For example, it’s common to call 
something that might fail and have a good reason to ignore whether it failed or 
not. The idiom we are discussing requires treating those functions differently 
from ones that return void. If you refactor so that a function now returns an 
ignorable return value you need to visit all the call sites using return and 
change them into the two line form.

- It works for return, but not for break. I’ve seen at least one case where 
someone filled a switch statement with return statements instead of break 
statements so they could use this more-terse form. Now if we want to add one 
line of code after that switch, we need to convert all those cases into 
multi-line statements with break.

- Unless it’s mandatory, it’s a case where two programmers can make different 
style choices. If both forms are acceptable, then it introduces a little bit of 
disorder into our code.

One thing I like about it is that since “pass along the return value from this 
inner function to this outer function” can be written this way, the code can 
then survive refactorings where both the inner and outer functions might gain 
or lose the return value.

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


[webkit-dev] Rename AtomicString to AtomString

2019-01-30 Thread Darin Adler
So, did we reach consensus that we should rename AtomicString to AtomString?

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


Re: [webkit-dev] Compile error in WebGL2RenderingContext.cpp

2019-01-17 Thread Darin Adler
Vector’s inline capacity feature was originally created as an alternative to 
variable length arrays for most of the purposes people would want to put them.

Imagine, for example, that you need a variable length buffer of characters that 
is almost always going to be less then 32 bytes. You write this:

 Vector buffer;

You can then grow the buffer to whatever size you need. If it’s less than 32 
bytes then it uses space on the stack, and if more than 32 bytes it uses space 
on the heap.

The reason this is better than variable length arrays is that stack size often 
has a inflexible total limit, so it’s not OK in general to use an arbitrary 
amount of stack. The vector inline capacity allows us to easily use up to a 
fixed amount of stack, but then still correctly handle unusually large requests 
by spilling go the heap.

For that WebGL2 code the idiom would be something like this:

Vector parameters;
parameters.grow(numValues);

Not sure we have good Vector inline capacity documentation, but that’s a basic 
primer.

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


Re: [webkit-dev] the name "AtomicString"

2018-12-19 Thread Darin Adler
Seems to me we could fix the current problem by renaming from AtomicString to 
AtomString without causing any new problem.

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


Re: [webkit-dev] 'final' class specifier and 'final' method specifier

2018-12-19 Thread Darin Adler
Let’s be clear about what we are discussing.

The choice is not be between “final” and “override”.

The choice is between “final override”, “override final”, and “final” for 
functions which are both overrides and final.

— Darin

Sent from my iPhone

> On Dec 19, 2018, at 12:27 PM, Michael Catanzaro  wrote:
> 
>> On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev  
>> wrote:
>> Adding override to method which already has final specifier doesn't affect 
>> anything,
>> because both final and override may ony be used on virtual methods
> 
> FWIW I prefer override because it's much more clear what that keyword is used 
> for.
> 
> Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] 'final' class specifier and 'final' method specifier

2018-12-19 Thread Darin Adler
> On Dec 19, 2018, at 1:52 AM, Fujii Hironori  wrote:
> 
> I'd like to change this because 'final' doesn't necessarily imply
> 'override'. See the following stackoverflow:
> https://stackoverflow.com/questions/29412412/does-final-imply-override

I’d be happy to require both final and override if there was any benefit to 
doing so.

I read it fairly carefully, and I don’t think it says anything meaningful for 
us. Maybe we can find a real example in WebKit code where there’s a “final” and 
if we added “override”, it would have caught a mistake that “final” won’t catch?

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


Re: [webkit-dev] the name "AtomicString"

2018-12-18 Thread Darin Adler
The name “AtomicString” was inspired by the term of art, “atom”, traditionally 
used in at least some programming language implementations for what I see now 
is often called interned strings. You’ll see a mention of that term in the 
article https://en.wikipedia.org/wiki/String_interning in the context of ML 
along with some other terms used for this such as “symbol”.

I’ve gotten used to the name AtomicString over the years, but I wouldn’t 
strongly object to changing it if other programmers are often confused by it’s 
similarity to the term “atomic operations”.

A mild objection I have to the term “interned string” is that the term 
“interned” is not really a normal English word; I wasn’t familiar with that 
jargon when we named AtomicString and I am still not entirely thrilled with it. 
I think that specific term comes from the LISP intern function and is familiar 
to programmers largely because of its use in Java, .NET, and some other modern 
programming languages and libraries; I had encountered the technique many times 
over the years without ever hearing the word “interning” and don’t find the 
jargon entirely logical.

Some people might suggest using the term “flyweight string” instead 
https://en.wikipedia.org/wiki/Flyweight_pattern and I’m not sure which I’d 
prefer. Maybe there’s another obvious name?

Apparently in Delphi’s DWScript they called it “unified string” 
https://www.delphitools.info/2013/06/17/string-unification/ but in the article 
I cited they are chided for not calling it an interned string.

— Darin

PS: I do love loudly declaiming, “Atomic string, the string of tomorrow!” but I 
can get over it.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] WebCorePrefix.h vs. config.h

2018-12-11 Thread Darin Adler
> On Dec 9, 2018, at 10:34 PM, Fujii Hironori  wrote:
> 
> MSVC has /FI option.
> 
>   /FI (Name Forced Include File) | Microsoft Docs
>   
> https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-file?view=vs-2017
> 
> Unfortunately, it seems that MSVC's precompiled header needs to be included 
> explicitly.
> 
>   /Yu (Use Precompiled Header File) | Microsoft Docs
>   
> https://docs.microsoft.com/en-us/cpp/build/reference/yu-use-precompiled-header-file?view=vs-2017

So this seems like the main potential obstacle. We can force an include of a 
prefix header, or precompile a header, but not both, with the Microsoft 
compiler.

If we deal with this, then I think we could get rid of “config.h”; every other 
platform can handle a prefix header.

Did I understand that right?

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


Re: [webkit-dev] WebCorePrefix.h vs. config.h

2018-12-08 Thread Darin Adler
OK, here’s my answer after thinking on it a bit:

Best would be to eliminate “config.h”: Change “config.h” into an empty file 
first, then remove all “config.h” includes, and then remove the file. But to do 
that, we need to make sure every build system for WebKit supports prefix 
headers. I don’t know how close to that we are. Maybe close? How can we quickly 
find out?

Lacking that, I think we can and should change “config.h” so it’s just an 
include of “WebCorePrefix.h”, or the other way around. I think it would be 
valuable to keep the feature where we try to catch cases where we forget to 
include “config.h”, on the platforms that use a prefix header, for the benefit 
of the platforms that do not. That might mean small complexity remains and one 
file won’t literally just be a trivial include of the other.

I suppose it’s also important to verify that there is no benefit to 
precompiling less than all of what “config.h” includes.

— Darin

PS: I don’t think we know that there is only one configuration that needs 
“config.h”. That second code snippet from your original message, Alexey, was 
only relevant for platforms that are trying to support macOS without prefix 
headers, and there could be any number of non-macOS cases. (And that include 
seems like a relatively recent change done by someone who didn’t fully 
understand the original scheme.)
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] WebCorePrefix.h vs. config.h

2018-12-08 Thread Darin Adler
> On Dec 8, 2018, at 2:56 PM, Darin Adler  wrote:
> 
>> On Dec 7, 2018, at 2:44 PM, Alexey Proskuryakov  wrote:
>> 
>> only keep config.h just to include WebCorePrefix for the lone build scenario 
>> where that's needed, and to undef new/delete?
> 
> I think the answer likely lies here: What is this lone build scenario where 
> config.h is needed?

I guess it’s CMake with Unix makefiles. Question for our CMake on Unix experts: 
Can we get CMake with Unix makefiles to support prefix headers?

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


Re: [webkit-dev] WebCorePrefix.h vs. config.h

2018-12-08 Thread Darin Adler
Useful background exists in Wikipedia: 
https://en.wikipedia.org/wiki/Prefix_header 
 and 
https://en.wikipedia.org/wiki/Precompiled_header 
 are relevant.

Alexey, perhaps you know all of this already, but here’s how I understand the 
intention behind having both of these files.

The “WebCorePrefix.h” file is a prefix header. We put things here that we want 
defined everywhere in the project.

The “config.h” file is a workaround for build systems that don’t support a 
prefix header. It’s inspired by the “config.h” files used in build systems 
based on autoconf, and was originally intended to keep the WebKit project 
compatible with them.

The file is unnecessary for builds systems like Xcode that support prefix 
headers.

Once we decided to use “config.h” as a “pseudo prefix header” we still decided 
to have a real prefix header under the Xcode build system, at least so we could 
precompile it. Ideally, under the Xcode build system, including “config.h” 
should have no effect at all other than participating in the check to help us 
ensure we didn’t forget to include it for the benefit of other build systems. 
We wanted to cleverly devise the contents of the prefix header so it’s easy to 
catch a mistake where we forget to include “config.h” somewhere; we’d like to 
get compile errors if we forget in most cases.

Maybe we don’t need both of these any more. Two possibilities occur to me:

- If we don’t need to support systems without support for prefix headers, we 
can eliminate “config.h”, which would be nice since we can streamline all our 
source files by removing the include for it.

- If we can get fast compilation without precompiling a header, then we don’t 
need to use a prefix header.

However, if we need support for systems without prefix headers and we need to 
use a prefix header for precompilation, then I do think we need to keep both of 
these. Their names or contents could change, but I think would still need two 
separate headers.

It would be great to “purify” any strange properties that these headers have 
accumulated. There should not be meaningful content repeated in both places. 
Ideally, content that needs to be included everywhere would be in a single 
place, perhaps a third header appropriately included by these two, and the 
prefix header and “pseudo prefix header” would just contain the tricks used to 
check for their proper use.

A corollary to this is that we should resist the urge to put platform-specific 
things into the prefix header just because it happens to be used on those 
platforms and no others. So perhaps there are Cocoa-specific things in 
WebCorePrefix.h that should instead be in a common place shared by both.

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


Re: [webkit-dev] WebCorePrefix.h vs. config.h

2018-12-08 Thread Darin Adler
> On Dec 7, 2018, at 2:44 PM, Alexey Proskuryakov  wrote:
> 
> only keep config.h just to include WebCorePrefix for the lone build scenario 
> where that's needed, and to undef new/delete?

I think the answer likely lies here: What is this lone build scenario where 
config.h is needed?

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


Re: [webkit-dev] Clarifying feature flags

2018-11-07 Thread Darin Adler
A while ago Kentaro Hara did some IDL keyword cleanup.

One of the best things he did was to make a single file with a list of all the 
keywords. In that case he was able to set things up so that if the file was 
inaccurate the build would fail; not sure if that’s practical for the various 
feature flags, but it’s helpful to have a single place. Here’s a link to an 
interesting WebKit mailing list message about one of the steps in that 
>.

I’d love us to find a similar way to “manage” and make it so someone can look 
over the entire list of feature flags and conditionals so we can spot problems 
easily.

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


Re: [webkit-dev] Rename wtf/unicode/UTF8.h

2018-10-31 Thread Darin Adler
> On Oct 31, 2018, at 7:52 AM, Konstantin Tokarev  wrote:
> 
> 31.10.2018, 05:18, "Fujii Hironori"  >:
>> wtf/unicode/UTF8.h is conflicting with ICU header in MSVC builds. I'd like 
>> to rename wtf/unicode/UTF8.h to wtf/unicode/WTFUTF8.h.
>> Any suggestion?
> 
> What about Unicode.h or UnicodeHelpers.h? UTF8.h deals with UTF16 as well


While I don’t love other of those names, I do like the idea of avoiding awkward 
“WTF” in the filename.

I don’t think it’s right to say “deals with UTF16”; this header contains only 
functions about dealing with UTF-8 and converting UTF-8 to and from other 
encodings (and yet those other encoding include UTF-16).

With a few seconds thought I am thinking that maybe UTF8Conversion.h or 
UTF8Transcoding.h are possible better ideas for new names. Neither is 
completely accurate. If we were going to add the word “helpers” than I would 
say UTF8Helpers.h, but I really don’t like those kinds of word in header names 
(“utilities”, “helpers”, “functions”, “classes”).

A separate issue once we rename: the header is also pretty old and crufty. 
Eventually we might want to remove or refine the functions in here. Not sure 
how widely they are used.

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


Re: [webkit-dev] node-jsc: A node.js port to the JavaScriptCore engine and iOS

2018-09-23 Thread Darin Adler
> On Sep 23, 2018, at 1:34 PM, Koby Boyango  wrote:
> 
> I will merge your changes to my fork

Would you be willing to focus on upstreaming first, instead? That would also 
get you the latest improvements, but in a more sustainable way.

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


Re: [webkit-dev] Offline Assembler build step always computes hashes even when nothing changes

2018-09-17 Thread Darin Adler
I don’t know. 

Sent from my iPhone

> On Sep 17, 2018, at 7:49 AM, Filip Pizlo  wrote:
> 
> 
> 
>> On Sep 16, 2018, at 8:48 PM, Darin Adler  wrote:
>> 
>>> On Sep 16, 2018, at 5:59 PM, Filip Pizlo  wrote:
>>> 
>>> Which offline assembler build step are you referring to?
>> 
>> The one that is the “Offline Assembler” target in Xcode, which runs this 
>> command:
>> 
>> ruby JavaScriptCore/offlineasm/asm.rb 
>> JavaScriptCore/llint/LowLevelInterpreter.asm 
>> "${BUILT_PRODUCTS_DIR}/JSCLLIntOffsetsExtractor” LLIntAssembly.h
>> 
>> For a “nothing rebuild” of all of WebKit and all of Safari for iOS on my 
>> iMac, it takes about 10 seconds out of a 30 second total “build" time.
>> 
>> Looking more carefully at the build log now, it seems that recompiling 
>> LLIntOffsetExtractor.cpp is also taking multiple seconds. Not executing 
>> generate_offset_extractor.rb, but compiling the output.
> 
> Does every build that you do rebuild LLIntOffsetExtractor.cpp?  Including a 
> clean build?
> 
> -Filip
> 
>> 
>> — Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Offline Assembler build step always computes hashes even when nothing changes

2018-09-16 Thread Darin Adler
> On Sep 16, 2018, at 5:59 PM, Filip Pizlo  wrote:
> 
> Which offline assembler build step are you referring to?

The one that is the “Offline Assembler” target in Xcode, which runs this 
command:

ruby JavaScriptCore/offlineasm/asm.rb 
JavaScriptCore/llint/LowLevelInterpreter.asm 
"${BUILT_PRODUCTS_DIR}/JSCLLIntOffsetsExtractor” LLIntAssembly.h

For a “nothing rebuild” of all of WebKit and all of Safari for iOS on my iMac, 
it takes about 10 seconds out of a 30 second total “build" time.

Looking more carefully at the build log now, it seems that recompiling 
LLIntOffsetExtractor.cpp is also taking multiple seconds. Not executing 
generate_offset_extractor.rb, but compiling the output.

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


[webkit-dev] Offline Assembler build step always computes hashes even when nothing changes

2018-09-16 Thread Darin Adler
I noticed that the “Offline Assembler” build step was taking between 5 and 30 
seconds every time I build. Really stands out in incremental builds. I realized 
that this step does not do any dependency analysis. Every time, it builds a 
hash of the input to see if it needs to recompute the assembly.

That’s probably not the best pattern; normally we like to use file modification 
dates to avoid doing any work when files haven’t changed.

Is there someone who can help me fix this so we get faster incremental builds?

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


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

2018-09-01 Thread Darin Adler
> On Sep 1, 2018, at 5:14 PM, 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.

Yes.

We knew this was going to happen when evaluated the proposal before we switched 
to unified sources. I believe you can find some discussion of it in webkit-dev.

> How can we solve this?

I don’t think we should try to solve it.

To me at the moment, this seems to be a minor irritation. Even without unified 
sources, it’s common to get includes wrong for platforms other than the one you 
are testing on and find this out only when building on EWS.

I would be OK having an EWS server that builds various platforms without 
unified sources, but while it might help it might also hurt, adding more EWS 
results to interpret for each patch, and also finding theoretical problems that 
don’t affect any platform.

If there’s a tool that can figure out what files need to be included by 
analyzing source code, and that works well enough to be practical, I’d love to 
arrange it so that we can use that instead of having programmers have to write 
their own include statements. I’ve long been interested in 
> and 
wondered whether we could use it, but it sort of does the opposite, and the 
“multiple platforms” problem could well make even that tool impractical for us.

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


Re: [webkit-dev] Freenode spam counter-measure

2018-08-06 Thread Darin Adler
> On Aug 6, 2018, at 5:22 AM, Konstantin Tokarev  wrote:
> 
>>>  On Aug 5, 2018, at 2:38 AM, Philippe Normand  wrote:
>>> 
>>>  Can one of the #webkit admin please set the +r mode on? This would help 
>>> reducing spam. Only messages from registered nicks would come through.
>> 
>> I tried this by typing this:
>> 
>> /msg ChanServ set #webkit restricted on
> 
> This command works differently from +r: it forbids join for people who are 
> not in access list, instead of simply forbidding unregistered users.

What’s the ChanServ function for setting +r?

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


Re: [webkit-dev] Freenode spam counter-measure

2018-08-05 Thread Darin Adler
> On Aug 5, 2018, at 2:38 AM, Philippe Normand  wrote:
> 
> Can one of the #webkit admin please set the +r mode on? This would help 
> reducing spam. Only messages from registered nicks would come through.

I tried this by typing this:

/msg ChanServ set #webkit restricted on

But ChanServ replied:

ChanServ: You are not authorized to perform this command.

I wonder who is authorized!?

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


Re: [webkit-dev] Proposal: Not supporting x86 w/o SSE2

2018-07-29 Thread Darin Adler
Sounds good.

There are clients of WebKit outside web browsing. A significant client like 
that on Windows at Apple is iTunes. I checked 
 and Windows versions of iTunes require 
a processor with support for SSE2, so clarifying WebKit’s lack of support won’t 
be a problem there.

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


Re: [webkit-dev] Objective-C code in libwebrtc already assuming ARC?

2018-06-06 Thread Darin Adler
> On Jun 6, 2018, at 9:39 AM, Dan Bernstein  wrote:
> 
> libwebrtc.xcconfig sets CLANG_ENABLE_OBJC_ARC to YES for the libwebrtc 
> target. Since RTCVideoCodecH264.mm is part of that target the file is 
> compiled with ARC.

OK, great. That’s what I missed.

I’ll get rid of the stray -fobjc-arc in 
ThirdParty/libwebrtc/libwebrtc.xcodeproj/project.pbxproj for 
voice_processing_audio_unit.mm as a cleanup step.

Should we also set it in ThirdParty/libwebrtc/Configurations/Base.xcconfig or 
is there a good argument against doing that? I am considering setting it there 
and removing it from libwebrtc.xcconfig since eventually all the Base.xcconfig 
will have it. Or maybe we generally keep all the Base.xcconfig files in sync 
with each other?

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


Re: [webkit-dev] Objective-C code in libwebrtc already assuming ARC?

2018-06-06 Thread Darin Adler
> On Jun 6, 2018, at 9:28 AM, Darin Adler  wrote:
> 
> best keep working with manual retain and release

best kept working

> a non-tribal amount of code that already seems to assume ARC

a non-trivial amount

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


[webkit-dev] Objective-C code in libwebrtc already assuming ARC?

2018-06-06 Thread Darin Adler
Hi folks.

As some of you have probably noticed, I’ve begun making changes with the goal 
of preparing us to move most Objective-C code in WebKit to ARC. In doing so, I 
have been exploring the existing code in the tree and the various projects in 
our source tree. The nine projects with configuration files that I have looked 
at are:

JavaScriptCore, ANGLE, libwebrtc, WebCore, WebCore/PAL, WebInspectorUI, WebKit, 
WebKitLegacy/mac, and bmalloc.

These projects all currently have CLANG_ENABLE_OBJC_WEAK in their configuration 
files, and moving them to ARC involves adding CLANG_ENABLE_OBJC_ARC, and then 
doing lots of other work to ensure the projects still work properly, including 
possibly turning off ARC for certain source files that are best keep working 
with manual retain and release. Some of them such as ANGLE, bmalloc, and 
WebInspectorUI, have no Objective-C code, or almost none, so they should be 
easy to “convert".

But when investigating libwebrtc I discovered a non-tribal amount of code that 
already seems to assume ARC but to be compiled with ARC disabled. One example 
is these two methods:

-[RTCVideoEncoderFactoryH264 supportedCodecs]
-[RTCVideoDecoderFactoryH264 supportedCodecs]

If we are compiling this without ARC and executing calls to these methods, then 
I think we will have storage leaks.

As far as I can tell, the only source file in libwebrtc that is currently 
compiled with ARC enabled is voice_processing_audio_unit.mm but perhaps I am 
overlooking something.

Do we need to turn ARC on for the entire libwebrtc project?

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


Re: [webkit-dev] Build issues due to anonymous namespace

2017-12-03 Thread Darin Adler
> On Dec 3, 2017, at 12:16 PM, Filip Pizlo  wrote:
> 
> That also means not using static, for the same reason. FWIW, I think it’s a 
> good idea.

Maybe.

There is definitely no benefit in wrapping a class, structure, or type 
definition in an anonymous namespace. My comment was specifically about a 
structure.

For functions and global variables, static or anonymous namespace does have one 
remaining benefit. It’s a way to communicate to the compiler’s warning system 
and the -Wmissing-declarations warning that the function does not need to have 
been previously declared in a header. So, for WebKit, these would not mean “I 
can rely on the fact that this name is private to this file”. They would mean 
“I only intend to use this in this file and so did not declare it in a header 
and so please don’t warn me about the fact that I did not”.

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


Re: [webkit-dev] Build issues due to anonymous namespace

2017-12-03 Thread Darin Adler
I think it’s also worthwhile to remove the anonymous namespace wrapping each of 
these DestroyFunc structures when renaming them. Generally speaking, anonymous 
namespace doesn’t work when compilation units are arbitrary, since they say 
“limit this to one compilation unit”. So I’m not sure we should ever use them 
any more.

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


Re: [webkit-dev] Unified sources build errors when adding new source files

2017-11-21 Thread Darin Adler
Oh, I see. Errors in EWS in that bug. Looking.

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


Re: [webkit-dev] Unified sources build errors when adding new source files

2017-11-21 Thread Darin Adler
> On Nov 21, 2017, at 6:54 AM, Javier Fernandez  wrote:
> 
> It builds perfectly with the no-unify tag, but I produces totally
> unrelated errors when using the unified building. I wonder whether my
> patch could make those errors arise because of altering the building
> units (8 files, as far as I know) when adding the mentioned new entry to
> the WebCore/Source.txt file.

Seems likely. We could help you more if you gave specifics on the errors.

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


[webkit-dev] EWS queues seem stuck

2017-10-08 Thread Darin Adler
A couple of my patches have been sitting around all day and some of the EWS 
servers still say they are 15 patches behind. Are they stuck? Can someone bring 
them back to life?

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


Re: [webkit-dev] Tests failing because WPT images not served at correct URL?

2017-10-07 Thread Darin Adler
Youenn is helping me figure this out. My original guess about why it’s failing 
was probably wrong. You can follow along here 
>.

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


[webkit-dev] Tests failing because WPT images not served at correct URL?

2017-10-07 Thread Darin Adler
Look at the expected results here 
.
 The results show “expected" failures like this one:

"FAIL Image with usemap of #åωk should not match any of the areas null is not 
an object (evaluating 'element.parentElement’)”

But that failure is not happening because of a bug in WebKit. It’s happening 
because the test image, referenced by the test with the URL 
"/images/threecolors.png”, is in the directory 
"LayoutTests/imported/w3c/web-platform-tests/images”. That does not show up 
correctly in the web server at the path “/images/threecolors.png”. It seems 
that “/“ serves the contents of “LayoutTests“, not the contents of 
“LayoutTests/imported/w3c/web-platform-tests”.

Who set this up? How is it supposed to work? Is this a known problem with the 
way we run the web server for the Web Platform Tests? Can someone fix this? 
Should I file a bug about this at bugs.webkit.org instead of writing to the 
list about it?

— 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

2017-08-29 Thread 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

2017-08-29 Thread 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

2017-08-29 Thread 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


Re: [webkit-dev] What's the rationale for not including config.h in any header files?

2017-08-01 Thread Darin Adler
> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro <mcatanz...@igalia.com> wrote:
> 
> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler <da...@apple.com> wrote:
>> I don’t think we should add lots of includes of “config.h”, though. I think 
>> we can come up with something better.
> 
> Like what?

We should consider reducing the size of “config.h” and only use it for things 
that it’s really needed for, using more conventional header includes for other 
purposes.

Ideally we would eliminate “config.h” entirely and replace it with normal 
headers.

I definitely don’t think we need to use the same name, “config.h”, for 9 
separate headers that don’t have the same contents in 9 different directories 
in the overall WebKit source code. A risk of putting includes of “config.h” in 
header files is that it isn’t clear which of the “config.h” files will be 
included. If we aren’t obliged to use the name “config.h” because of autotools, 
then perhaps we can name these more in line with WebKit coding style, something 
like WebCoreConfiguration.h, but also strive to get rid of them entirely.

In projects not using autotools and using IDEs to build I often see a header 
referred to as the “prefix” that is automatically included by the IDE, with no 
#include statements at all in the source files. That’s used as a sort of 
substitute for #define statements on the command line and is what I used before 
I became familiar with autotools.

I am concerned to see there is also a header named “cmakeconfig.h” so maybe we 
do need “config.h”.

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


Re: [webkit-dev] What's the rationale for not including config.h in any header files?

2017-07-31 Thread Darin Adler
We originally adopted this “config.h” style to make WebKit buildable with 
autotools. Since that has not been a consideration for years I would be willing 
to abandon this and change how we do things.

I don’t think we should add lots of includes of “config.h”, though. I think we 
can come up with something better.

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


Re: [webkit-dev] Does someone know how to fix WTF::Function on Windows

2017-07-15 Thread Darin Adler
Right, that’s what I am looking to undo.

— Darin

Sent from my iPhone

> On Jul 15, 2017, at 12:28 PM, Chris Dumez <cdu...@apple.com> wrote:
> 
> I seem to remember I fixed it last time by calling the WTF::Function 
> constructor explicitly.
> 
> Chris Dumez
> 
>> On Jul 15, 2017, at 9:14 AM, Yusuke SUZUKI <utatane@gmail.com> wrote:
>> 
>> I'm not 100% confident, but can you try it `` instead?
>> 
>> Regards,
>> Yusuke Suzuki
>> 
>>> On Sun, Jul 16, 2017 at 1:13 AM, Darin Adler <da...@apple.com> wrote:
>>> Hi folks.
>>> 
>>> On Windows, WTF::Function doesn’t quite work right. Code that is something 
>>> like this:
>>> 
>>> void addCallback(WTF::Function<void()>&&);
>>> 
>>> void testFunction()
>>> {
>>> // ...
>>> }
>>> 
>>> void addTestFunction()
>>> {
>>> addCallback(testFunction);
>>> }
>>> 
>>> Leads to errors like this:
>>> 
>>> error C2664: 'void addCallback(WTF::Function &&)': cannot 
>>> convert argument 1 from 'void (__cdecl *)(void)' to 'WTF::Function>> (void)> &&'
>>> note: You cannot bind an lvalue to an rvalue reference
>>> 
>>> The problem might have something to do with cdecl vs. stdcall functions, 
>>> but I am not sure that is the problem. It could be some other problem with 
>>> how WTF::Function is written. Or it might even be a bug in the Visual 
>>> Studio compiler. Since I don’t have a Windows machine myself, I was trying 
>>> to use EWS to figure this out but that was slow. Then I tried using 
>>> http://webcompiler.cloudapp.net but I could not reproduce any error there 
>>> when I pasted in cut down code; it just compiled fine.
>>> 
>>> Is there someone who knows how to fix this?
>>> 
>>> Another way to put this is: We want to take off the explicit WTF::Function 
>>> conversions in functions like canUseWithReason in SimpleLineLayout.cpp, 
>>> Page::Page in Page.cpp, and Worker::Worker in Worker.cpp and have it still 
>>> compile and work on Windows. Other platforms seem to compile fine without 
>>> the explicit WTF::Function.
>>> 
>>> — 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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Does someone know how to fix WTF::Function on Windows

2017-07-15 Thread Darin Adler
Hi folks.

On Windows, WTF::Function doesn’t quite work right. Code that is something like 
this:

void addCallback(WTF::Function&&);

void testFunction()
{
// ...
}

void addTestFunction()
{
addCallback(testFunction);
}

Leads to errors like this:

error C2664: 'void addCallback(WTF::Function &&)': cannot 
convert argument 1 from 'void (__cdecl *)(void)' to 'WTF::Function 
&&'
note: You cannot bind an lvalue to an rvalue reference

The problem might have something to do with cdecl vs. stdcall functions, but I 
am not sure that is the problem. It could be some other problem with how 
WTF::Function is written. Or it might even be a bug in the Visual Studio 
compiler. Since I don’t have a Windows machine myself, I was trying to use EWS 
to figure this out but that was slow. Then I tried using 
http://webcompiler.cloudapp.net but I could not reproduce any error there when 
I pasted in cut down code; it just compiled fine.

Is there someone who knows how to fix this?

Another way to put this is: We want to take off the explicit WTF::Function 
conversions in functions like canUseWithReason in SimpleLineLayout.cpp, 
Page::Page in Page.cpp, and Worker::Worker in Worker.cpp and have it still 
compile and work on Windows. Other platforms seem to compile fine without the 
explicit WTF::Function.

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


[webkit-dev] Why does our std::optional lack the has_value() function?

2017-06-19 Thread Darin Adler
I noticed we don’t have has_value() in our version of std::optional. Does 
anyone know why?

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


[webkit-dev] Should we ever use std::function instead of WTF::Function?

2017-06-13 Thread Darin Adler
I’ve noticed many patches switching us from std::function to WTF::Function 
recently, to fix problems with copying and thread safety.

Does std::function have any advantages over WTF::Function? Should we ever 
prefer std::function, or should we use WTF::Function everywhere in WebKit where 
we would otherwise use std::function?

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


Re: [webkit-dev] WebCore/platform standalone library

2017-01-14 Thread Darin Adler
> On Jan 14, 2017, at 12:39 PM, Myles C. Maxfield  wrote:
> 
> However, a shared library would enforce layering naturally, whereas a static 
> library would need either an application to link to it and not to WebCore, 
> such as a unit test suite, or some out-of-band layering enforcement, like a 
> Python script.

I’m surprised to learn this. There is no way to build a static library that 
gives us linker errors if there are unresolved references?

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


Re: [webkit-dev] usage of auto

2017-01-12 Thread Darin Adler
We probably need to step away from mandating style for a while until we have 
more consensus.

I’m sad that we are so far away from that right now. I’ve found greatly 
increased use of auto during coding and refactoring that I am doing feels like 
it’s improving clarity quite at bit. From the point of view of project norms 
I’m a little disappointed that so many people see it so dramatically 
differently!

> On Jan 12, 2017, at 9:24 AM, Filip Pizlo  wrote:
> 
>> On Jan 12, 2017, at 08:54, Brady Eidson > > wrote:
>> 
>>> My take-away from this discussion so far is that there is actually very 
>>> little consensus on usage of auto, which means there’s probably very little 
>>> room for actual style guideline rules.
>>> 
>>> I think there are two very limited rules that are probably not 
>>> objectionable to anybody.
>>> 
>>> 1 - If you are using auto for a raw pointer type, you should use auto*
>>> 2 - If you are using auto in a range-based for loop for values that aren’t 
>>> pointers, you should use (const) auto&
> 
> In some cases you need a copy for the code to be correct. I understand why & 
> is often better for performance but there is a significant and dangerous 
> behavioral difference.
> 
> I agree with encouraging people to use auto& because it's usually ok, but I 
> disagree with mandating it because it's sometimes wrong. 


Lets talk about that specific point for a moment. Consider these two cases:

1) Iterating over a collection that is not being modified and operating on the 
objects inside the collection in a straightforward way. This idiom is 
incredibly common. Iterating over an entire collection is a pattern that occurs 
constantly in WebKit code, and we lean more toward the for loop than using 
lambdas for that kind of operation. In these cases, copying the items as we 
iterate is not valuable, often bad for efficiency, and a very easy mistake to 
make because of how auto works. I have fixed at least 20 examples, maybe as 
many as 100, of copying a RefPtr by using a for loop and so churning the 
reference count for no benefit. Using “auto” creates this problem in the first 
place, since it makes the copy of the RefPtr subtle, missed by many 
programmers. And “auto&” is a pretty good solution, I think better than writing 
RefPtr&. To me “auto&” is the way to say “operate on the members of this 
collection in place”, without the distraction of stating anything else about 
the iteration, such as the type of the iterator, for example, which you would 
see if the begin/end was written out without the use of a modern for loop or 
auto. The biggest downside of “auto&” as a solution is that the incorrect 
“auto” and the correct “auto&” are so subtly different that it’s easy to fail 
to spot incorrect code. The future version of C++ where you can do a 
non-copying for loop omitting any mention of the type will be even better, 
because “no type at all” is not easily confusable with the incorrect “auto”. In 
that new feature, the type is “auto&&” where not specified. I look forward to 
when we can use that feature.

2) The occasional for loops where copying each item in the collection is part 
of what we want to do. There are multiple options here. We could name the type 
in the for loop and count on people’s understanding that means copying. Or if 
we want to be more explicit, we could have the for loop use “auto&” and write 
the copying out explicitly as a separate statement. That would also work for 
moving. Less terse than doing the copying as a side effect of how the for loop 
itself is written, but perhaps much clearer because of how explicit it is. 
Since this is a tiny fraction of for loops I haven’t had to consider this style 
question often.

I personally don’t like using auto& when iterating a collection that contains a 
scalar such as an int or bool with a short name. But there is a change using 
auto& when iterating a collection containing a complex type that we want to 
copy might be a good style idea. Maybe even a good enough idea that  we some 
day would want to mandate it!

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


Re: [webkit-dev] [webkit-reviewers] usage of auto

2017-01-11 Thread Darin Adler
> On Jan 11, 2017, at 9:41 AM, Alexey Proskuryakov  wrote:
> 
> In a way, these are read-time assertions.

Exactly. A type name is a read-time assertion of the specific type that a 
variable has and “auto” is a read-time assertion that the type of the variable 
is the same as the type of the expression on the right.

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


Re: [webkit-dev] [webkit-reviewers] usage of auto

2017-01-11 Thread Darin Adler
OK, you didn’t convince me but I can see that your opinions here are strongly 
held!

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


Re: [webkit-dev] [webkit-reviewers] usage of auto

2017-01-10 Thread Darin Adler
> On Jan 10, 2017, at 10:17 PM, Filip Pizlo  wrote:
> 
> while (Arg src = worklist.pop()) {
> HashMap::iterator iter = 
> mapping.find(src);
> if (iter == mapping.end()) {
> // With a shift it's possible that we previously built 
> the tail of this shift.
> // See if that's the case now.
> if (verbose)
> dataLog("Trying to append shift at ", src, "\n");
> currentPairs.appendVector(shifts.take(src));
> continue;
> }
> Vector pairs = WTFMove(iter->value);
> mapping.remove(iter);
> 
> for (const ShufflePair& pair : pairs) {
> currentPairs.append(pair);
> ASSERT(pair.src() == src);
> worklist.push(pair.dst());
> }
> }

Here is the version I would write in my favored coding style:

while (auto source = workList.pop()) {
auto foundSource = mapping.find(source);
if (foundSource == mapping.end()) {
// With a shift it's possible that we previously built the tail of 
this shift.
// See if that's the case now.
if (verbose)
dataLog("Trying to append shift at ", source, "\n");
currentPairs.appendVector(shifts.take(source));
continue;
}
auto pairs = WTFMove(foundSource->value);
mapping.remove(foundSource);
for (auto& pair : pairs) {
currentPairs.append(pair);
ASSERT(pair.source() == source);
workList.push(pair.destination());
}
}

You argued that specifying the type for both source and for the iterator helps 
reassure you that the types match. I am not convinced that is an important 
interesting property unless the code has types that can be converted, but with 
conversions that we must be careful not to do. If there was some type that 
could be converted to Arg or that Arg could be converted to that was a 
dangerous possibility, then I grant you that, although I still prefer my 
version despite that.

You also said that it’s important to know that the type of foundSource->value 
matches the type of pairs. I would argue that’s even clearer in my favored 
style, because we know that auto guarantees that are using the same type for 
both, although we don’t state explicitly what that type is.

Here’s one thing to consider: Why is it important to see the type of 
foundSource->value, but not important to see the type of shifts.take(source)? 
In both cases they need to be Vector.

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


Re: [webkit-dev] [webkit-reviewers] usage of auto

2017-01-10 Thread Darin Adler
> On Jan 10, 2017, at 9:49 PM, Darin Adler <da...@apple.com> wrote:
> 
>> On Jan 10, 2017, at 9:46 PM, Simon Fraser <simon.fra...@apple.com> wrote:
>> 
>> auto countOfThing = getNumberOfThings();
>> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
>> assured at compile time if countOfThing is unsigned
> 
> I understand wanting to know, but I am not certain this is a bad thing.

Sorry, let me say something different, but related:

int countOfThing = getNumberOfThings();

Can’t tell from the above code if getNumberOfThings() returns int or unsigned.

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


Re: [webkit-dev] [webkit-reviewers] usage of auto

2017-01-10 Thread Darin Adler
> On Jan 10, 2017, at 9:46 PM, Simon Fraser  wrote:
> 
> auto countOfThing = getNumberOfThings();
> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
> assured at compile time if countOfThing is unsigned

I understand wanting to know, but I am not certain this is a bad thing.

> auto thingLength = getLengthOfThing();
> IntSize size(thingLength, 2); // Can’t tell by reading if thingLength is 
> LayoutUnit or float and thus truncated here.

Makes total sense to me. This is a bad pattern.

> Another common issue in code I’m not familiar with is something like:
> 
> auto fancyStyleThing = styleMagic.styleThingForDoohicky()
> 
> where it maybe obvious to the author what the type of fancyStyleThing is, but 
> without looking at StyleMagic::styleThingForDoohicky() I have no idea what it 
> is, and Xocde doesn’t help me. You argue above that maybe I don’t need to 
> know the exact type, but often I do if I’m trying to figure out how various 
> components relate to each other, rather than studying the logic of one 
> specific function.

If the type of fancyStyleThing was given here it would not tell you the type of 
styleThingForDoohicky; it would tell you what type we are assigning to but 
there could be a type conversion.

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


Re: [webkit-dev] WebCore/platform standalone library

2017-01-10 Thread Darin Adler
> On Jan 10, 2017, at 9:24 PM, Myles C. Maxfield  wrote:
> 
> We opted for WebCore to include files using “#include ” instead of 
> just including Foo.h.

Will this work without ForwardingHeaders on macOS and iOS?

Given that WTF chose , what is the reasoning for PAL choosing the 
all capital ?

> Similarly, we are putting symbols inside the PAL namespace, which is a child 
> of the WebCore namespace. Therefore, inside WebCore, you use PAL things by 
> specifying “PAL::Foo”.

Given that WebCore is built on top of PAL, it seems a little strange to put PAL 
“inside” WebCore, but I guess OK.

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


Re: [webkit-dev] Thread naming policy in WebKit

2017-01-05 Thread Darin Adler
I understand the appeal of “org.webkit” and structured names but personally I 
would prefer to read names that look like titles and are made up of words with 
spaces, like these:

“WebKit: Image Decoder”, rather than “org.webkit.ImageDecoder”.
“WebKit: JavaScript DFG Compiler” rather than “org.webkit.jsc.DFGCompiler”.

Not sure how well that would generalize to all the different names.

I like the idea of having a smart way of automatically making a shorter name 
for the platforms that have shorter length limits.

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


Re: [webkit-dev] Reducing the use of EncodedJSValue and use JSValue directly instead.

2017-01-03 Thread Darin Adler
> On Jan 3, 2017, at 2:33 PM, Mark Lam  wrote:
> 
> I propose that we switch to using JSValue directly where we can.

Sounds like a great idea to me.

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


Re: [webkit-dev] Status of GTK EWS bot?

2016-10-25 Thread Darin Adler
> On Oct 25, 2016, at 1:18 AM, Philippe Normand  wrote:
> 
> The GTK EWS is back online.

Thanks! I was able to use the EWS results this morning to improve my patch.

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


[webkit-dev] Status of GTK EWS bot?

2016-10-24 Thread Darin Adler
Hi folks.

As I am uploading patches to  
for EWS processing I noticed that all the other platforms said “#1”, but 
gtk-wk2 said #85.

Is this just the bot falling a bit behind because of load, or is there a more 
serious problem? This is important to me because without the bot it is highly 
likely my patch will break the build for GTK; I normally update the GTK 
bindings code after the build failures on EWS expose which functions need 
updating.

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


Re: [webkit-dev] Terminology for giving up ownership: take, release, move

2016-09-07 Thread Darin Adler
> On Sep 7, 2016, at 1:57 AM, Maciej Stachowiak  wrote:
> 
> perhaps a syntax like Ref x = notNull(move(refPtr)) could be made to work

It definitely can.

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


Re: [webkit-dev] Terminology for giving up ownership: take, release, move

2016-09-06 Thread Darin Adler
> On Sep 6, 2016, at 6:43 PM, Maciej Stachowiak  wrote:
> 
> RefPtr does also have regular release() though. I'm not sure if this is for a 
> practical reason or just no one has fixed it yet.

It’s still around until we finish getting rid of PassRefPtr, that’s all.

> A wacky solution, based on your suggestion for releaseImpl, would be to have 
> a nonNull method which asserts the pointer is not null and then returns a 
> self reference, so you'd do move(ref.nonNull()).

I don’t think we can do that. I don’t know how to change a RefPtr into a 
Ref& in C++ even though we know the underlying object layout is identical.

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


Re: [webkit-dev] Terminology for giving up ownership: take, release, move

2016-09-06 Thread Darin Adler
> On Sep 6, 2016, at 4:48 PM, Maciej Stachowiak  wrote:
> 
> STL smart pointers have a 0-argument reset for non-returning remove instead, 
> and convention seems to be to use swap() or move() for the returning remove 
> on a smart pointer. So an alternate possibility would be to use the above 
> convention for collections, but make smart pointers have no remove+get 
> operation at all.

Our modern smart pointers follow the standard library convention you mention 
above.

It’s the peculiar cases that need names, such as:

“assert this RefPtr is non-null to turn it into a Ref&&”, currently named 
releaseNonNull

“release the RefPtr inside the String”, currently named releaseImpl(), but 
another possibility would be to have impl() be RefPtr& instead of a 
StringImpl*, so you could do move(string.impl()) instead of 
string.releaseImpl().

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


[webkit-dev] Terminology for giving up ownership: take, release, move

2016-09-05 Thread Darin Adler
Hi folks.

WebKit has some critical functions that involve asking an object to give up 
ownership of something so the caller can take ownership.

In the C++ standard library itself, this is called move, as in std::move.

In WebKit smart pointers, we call this operation release, as in 
RefPtr::releaseNonNull and String::releaseImpl.

In WebKit collections, we call this operation take, as in HashMap::take and 
ExceptionOr::takeReturnValue.

The release vs. take terminology is distracting to my eyes. The verb “take" 
states what the caller wishes to do, and the verb “release” states what the 
caller wants the collection or smart pointer to do. My first thought was be to 
rename the take functions to use the word release instead, but I fear it might 
make them harder to understand instead of easier and clearly it would make them 
longer.

Does anyone have other ideas on how to collapse WebKit project terminology down 
so we don’t have three different single words that are used to mean almost the 
same thing?

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


  1   2   3   4   5   6   7   8   9   10   >