Enabling more new clang-tidy checks

[Julia Hansbrough]

Hi all,

I'm interested in enabling the following clang-tidy checks:

bugprone-redundant-branch-condition

This diagnostic finds condition variables in nested `if` statements that were *also* checked in an outer `if` statement and have not changed.

Such checks are at minimum redundant and may indicate some logic failure to the developer (why are they thinking a particular condition needs to be checked multiple times?).

It triggers 16 times in Chromium's codebase, in a variety of third_party libraries (angle, blink, swiftshader, icu), as well as chrome/renderer.

Auditing the sites, I found no false positives and it seemed like the code would be clearer and more self-evidently correct in every case if the redundant branch were removed (example, example, example).

bugprone-suspicious-realloc-usage

This diagnostic finds instances of realloc that take the form:

`p = realloc(p, size);`

The issue here is that, if realloc fails for any reason, there will can a memory leak—`p` becomes a null pointer but the memory it points to is not deallocated.  (This is simple to fix with the form `g = realloc(p, size);`.)

This check triggers 22 times in Chromium, primarily in various third_party libraries (example, example, example).  I didn't find any false positives; it seems the check is relatively narrowly tailored to catch this specific type of misuse.

Given both of these warnings trigger relatively infrequently but catch real bugs/problems I'd like to enable them both.

The CL for these changes is here & I welcome any feedback.  Thanks!

[Alexei Svitkine]

Your realloc example links are the ones for the first analyzer.

For the first case, in this example you linked, isn't `visit` being updated in the loop?

--
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 visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADRQns5jTdfw5Dz4EQQHDGA8bOYhVAyyAX3d5wNo1-Aj_6p5jA%40mail.gmail.com.

[David Baron]

In the second linked example my intuition is that it's the *outer* stop_on_error test that's incorrect, and that there may be a real bug in the code rather than just a redundant check.  (I made a CL to ask the original author.)

I think that's perhaps an even stronger argument for enabling the check, since I think it might be finding a real bug and not just redundancy.

-David

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKFU2SA3EFhec0Fqh5y5D%3DJ4ikzeHCM7WNZpSN25BLP2%2Bf91vQ%40mail.gmail.com.

[Yoshisato Yanagisawa]

Hi, for the ease of writing a long message, let me reply here.

I am not the original author.  As you can see in the code history, the code has been imported from WebKit, and the original author should be Alex (https://github.com/WebKit/WebKit/commit/d7bfee949b6c3735485bb5c0795be3b931df723a#diff-b5344ba463834c773fff86fb57d9ad6031b91cf6dd7bbce90b3c50a26f78fa0b).

I understand that the CL implements the ISO-2022-JP decoder explained in https://encoding.spec.whatwg.org/#iso-2022-jp-decoder.

It is used in Step 4 of https://encoding.spec.whatwg.org/#concept-encoding-process.

You are asking to change the way to handle second_prepended_bytes_, which is set at:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/text_codec_cjk.cc;l=780;drc=69cb4a24bbe798ac9d4d7bb476a91fb1763df562

I understand prepended_bytes_ and second_prepended_bytes_ are come from Step 8 of https://encoding.spec.whatwg.org/#ref-for-iso-2022-jp-decoder-escape

Instead of restoring to the queue, it is represented as prepended_bytes_ and second_prepended_bytes_.

Therefore, when second_prepended_bytes_ is parsed, it might be parsed the same as prepended_bytes_ as far as I understand from 12.2.1. ISO-2022-JP decoder section.

Then, we may not need to exclude the case when stop_on_error is set.  (Note that stop_on_error should be "error mode" == fatal on the spec)

Also, I checked https://bugs.webkit.org/show_bug.cgi?id=216168, but I failed to understand why second_prepended_bytes_ should be handled differently from prepended_bytes_.

Then, I guess removing the `stop_on_error` check might be more aligned with the specification.

What do you think?

2025年5月31日(土) 1:09 David Baron <dba...@chromium.org>:

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAG0MU3j%2BoaJDzgK-6Dv5EtwUHt%3Dq6vfHUxqLd95-iiBrKFdbtw%40mail.gmail.com.