Enabling bugprone-assert-side-effect

[George Burgess]

Hello friends! 

I'd like to propose that we enable the bugprone-assert-side-effect clang-tidy lint. The evaluation doc I have is here.

I think the key points for discussion are:

- Do we want this on at all?

- If enabled, should we also have it treat all function calls as non-const?

Based on the previous discussion, my thoughts are:

- Despite only 1 of the 11 occurrences of this (with CheckFunctions disabled) being true-positives, I expect that most accidental side-effects are discovered in postsubmit and fixed. The primary benefit of this lint will be surfacing issues earlier. 

- Enabling the non-const function call detection has us flag 15K DCHECKs rather than 11. I estimate the flaggable number of DCHECKs in Chrome to be around 90K (see the doc for why :) ). We can probably chip away at this by adding ignored function regexes, but these also reduce the chances that we'll catch a real issue, since these regexes will have to be quite generic (e.g., does the function name contain "is"?)

Given the very low false-positive rate (10/90K DCHECKs across Chromium), and given that flagging this kind of issue pre-commit is much better than only discovering it in postsubmit, I'm in favor of adding this lint.

Feedback is appreciated!

Thanks,

George

[Peter Kasting]

On Thu, Mar 10, 2022 at 10:01 AM 'George Burgess' via cxx <c...@chromium.org> wrote:

- Do we want this on at all?

IMO, yes.

 

- If enabled, should we also have it treat all function calls as non-const?

It would be sort of nice in theory, but I suspect the additional utility isn't worth the (code health project to enable) + (friction of having to avoid this pattern).  As you note in the doc, for example, this basically requires people to replace

DCHECK(Foo::GetInstance());

Foo::GetInstance()->...

with

auto* foo = Foo::GetInstance();

DCHECK(foo);

foo->...

...which I think is a good change, but not worth forcing people to do.  This also promotes making Is...() and Has...() sorts of functions const, which I have the same feelings about.

So my suggestion is to enable without this config and take no further action in the future.

PK

[George Burgess]

Thanks for the feedback! I'm happy with what you're proposing. :)

I have a CL to land the check here. I'll wait until Thursdayish to land it, so other folks have time to voice any concerns/thoughts.

George

[George Burgess]

I hear no voices of opposition, so I'll land the CL to enable the check.

Thanks, all!

[Nico Weber]

Looks fine to me too fwiw (agreed with pkasting's replies).

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BrzOEnW3a4A6RW8wVbUYZHU59si_yEiyVL8Ggh7itEPd2i65w%40mail.gmail.com.

[dan...@chromium.org]

Thank you George!

--