I was looking at clang-tidy and include-what-you-need support in CMake. Clang 
currently won’t compile WebKit on Windows but I was going to look into that as 
well so those tools could end up running.

Happy to provide some patches if it bears fruit.

From: webkit-dev [mailto:webkit-dev-boun...@lists.webkit.org] On Behalf Of 
Maciej Stachowiak
Sent: Friday, April 28, 2017 1:11 PM
To: JF Bastien <jfbast...@apple.com>
Cc: webkit-dev@lists.webkit.org
Subject: Re: [webkit-dev] !!Tests for equality comparison



Sent from my iPhone

On Apr 28, 2017, at 1:00 PM, JF Bastien 
<jfbast...@apple.com<mailto:jfbast...@apple.com>> wrote:

On Apr 28, 2017, at 12:12, Maciej Stachowiak 
<m...@apple.com<mailto:m...@apple.com>> wrote:


Here's some comments in the other direction:

- If there are times we recommend x != 0 instead of !x, it should maybe be 
based on whether the condition is better expressed as "not zero" or "false". In 
the numTestsForEqualityComparison, that's clearly a "not zero" check given the 
naming of the variable. This could be addressed by removing zero/non-zero from 
the list with true/false and null/non-null instead of making a carve-out based 
on type.

- Allowing both forms for zero/non-zero comparisons would be unfortunate. We 
have style guidelines to avoid tiny inconsistencies like this. So if the 
guideline changes, it should be to *require* == 0 or != 0 for numeric 
comparisons to 0, not merely allow it. Proliferating both styles would be sad.

- If we adopted the new rule, it would be slightly sad that a bunch of old code 
doesn't follow it. Changing it all at once would be needless churn, but we'll 
end up with a lot of code in both styles, partly defeating the consistency 
benefits of having a style guideline at all. This is sort of a general issue 
with any change to the coding style guidelines. If we change the guideline for 
a frequently used construct, the benefit has to be really high to account for 
the fact that we'll have many years of inconsistency. Note that the guidelines 
are mainly for the benefit of people reading the code, not writing, and 
inconsistent style may be worse than consistently using a slightly worse form.

- The style checker wouldn't be able to check the rule since it's not smart 
enough to tell if you are doing a null check, a false check or a zero check. (I 
am not sure if it enforces the current rule.)

That’s kind of a sad reason though. If we think it’s really worth it, we can 
move to a clang-based approach. It’ll definitely be way more powerful than 
regular expressions. I really liked how That Other Browser did 
this<https://chromium.googlesource.com/chromium/src/tools/clang/+/master> (hook 
into clang for extra diagnostics, and you also get rewriting tools for Free™).

I listed that reason last intentionally. I don't think it's the most important. 
That said, I like the idea of a smarter style checker based on clang.





I don't actually have a strong opinion on the substance of the rule, either 
version seems fine to me if we were starting from a blank slate. I'm not 
entirely sure why the rule ended up that way in the first place. But I wanted 
to note these as things to think about.

Regards,
Maciej


On Apr 28, 2017, at 1:00 AM, Keith Miller 
<keith_mil...@apple.com<mailto:keith_mil...@apple.com>> wrote:

Is there any opposition to relaxing this rule? Speak now or forever hold your 
piece! (not really but I would be curious to hear opposition).

Cheers,
Keith

On Apr 27, 2017, at 10:32 PM, Carlos Garcia Campos 
<carlo...@webkit.org<mailto:carlo...@webkit.org>> wrote:

El jue, 27-04-2017 a las 16:06 -0700, JF Bastien escribió:

Hello C++ fans!

The C++ style check currently say:
Tests for true/false, null/non-null, and zero/non-zero should all be
done without equality comparisons

I totally agree for booleans and pointers… but not for integers. I
know it’s pretty much the same thing, but I it takes me slightly
longer to process code like this:

int numTestsForEqualityComparison = 0:
// Count ‘em!
// …
if (!numTestsForEqualityComparison)
  printf(“Good job!”);

I read it as “if not number of tests for equality comparison”. That's
weird. It takes me every slightly longer to think about, and I’ve
gotten it wrong a bunch of times already. I’m not trying to check for
“notness", I’m trying to say “if there were zero tests for equality
comparison”, a.k.a.:

if (numTestsForEqualityComparison == 0)
  printf(“Good job!”);

So how about the C++ style let me just say that? I’m not suggesting
we advise using that style for integers everywhere, I’m just saying
it should be acceptable to check zero/non-zero using equality
comparison.

I agree, it's also quite confusing when using strcmp, because !strcmp
means the strings are equal. It's not only more difficult to read, I've
seen patches with wrong strcmp checks because of that.

I also think this could be solved by #defining a success a C call positive 
result that is 0 (e.g. CCallSuccess), regardless of the choice we make here.





!!Thanks (i.e. many thanks),

JF

p.s.: With you I am, fans of Yoda comparison, but for another day
this will be.
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org<mailto:webkit-dev@lists.webkit.org>
https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org<mailto:webkit-dev@lists.webkit.org>
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org<mailto:webkit-dev@lists.webkit.org>
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org<mailto: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

Reply via email to