Enabling bugprone-assert-side-effect

21 views
Skip to first unread message

George Burgess

unread,
Mar 10, 2022, 1:01:43 PMMar 10
to cxx
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

unread,
Mar 10, 2022, 8:53:18 PMMar 10
to George Burgess, cxx
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

unread,
Mar 14, 2022, 6:29:49 PMMar 14
to Peter Kasting, cxx
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

unread,
Mar 17, 2022, 2:33:39 PMMar 17
to Peter Kasting, cxx
I hear no voices of opposition, so I'll land the CL to enable the check.

Thanks, all!

Nico Weber

unread,
Mar 17, 2022, 4:05:45 PMMar 17
to George Burgess, Peter Kasting, cxx
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

unread,
Mar 17, 2022, 4:06:07 PMMar 17
to George Burgess, Peter Kasting, cxx
Thank you George!

--
Reply all
Reply to author
Forward
0 new messages