1468811 - Re-enable some clang-tidy checks that we seem to have lost.

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Jan Keromnes [:janx]]

Currently, there are a few types of `clang-tidy` code defects that `./mach static-analysis check` doesn't catch, but I think it should catch them:

- clang-analyzer-security.insecureAPI.gets
- performance-faster-string-find
- performance-implicit-conversion-in-loop

Source: https://github.com/mozilla-releng/services/issues/1166#issuecomment-397387354

Comment 1

[Jan Keromnes [:janx]]

Update: clang-analyzer-security.insecureAPI.gets is actually deprecated now (the `gets` function was removed from the standard in 2011, so it doesn't even compile anymore).

I'll add a comment in our config file anyway, so that we don't re-try adding it in the future.

Comment 2

[Phabricator Automation]

Attachment: Bug 1468811 - Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml. r=sylvestre

Comment 3

[Phabricator Automation]

Attachment: Bug 1468811 - Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml. r=sylvestre

Comment 4

[Jan Keromnes [:janx]]

Attachment: Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml.

Comment 5

[Jan Keromnes [:janx]]

Attachment: Re-enable some performance clang-tidy checks.

Comment 6

[Jan Keromnes [:janx]]

The check "performance-implicit-conversion-in-loop" doesn't actually in clang-tidy 5.0 (thanks Sylvestre for finding this out!)

So I've tweaked these tests to fail if a check doesn't actually exist, and it now fails with:

> 0:06.05 ERROR: clang-tidy checker performance-implicit-conversion-in-loop doesn't exist in this clang-tidy version.

Comment 7

[Jan Keromnes [:janx]]

Attachment: Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml.

Comment 8

[Jan Keromnes [:janx]]

Attachment: Re-enable clang-tidy check performance-faster-string-find.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Re-enable clang-tidy check performance-faster-string-find.

Comment 10

[Jan Keromnes [:janx]]

Attachment: Fail static-analysis autotest when a clang-tidy check doesn't exist.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8985717 [details] [diff] [review]
Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml.

thanks!

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8985719 [details] [diff] [review]
Re-enable clang-tidy check performance-faster-string-find.

Could you please update the title before you commit?
The patch is doing more than just "faster string"

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8985720 [details] [diff] [review]
Fail static-analysis autotest when a clang-tidy check doesn't exist.

Great, thanks for doing that !

Comment 14

[Andi [:andi]]

Comment on attachment 8985719 [details] [diff] [review]
Re-enable clang-tidy check performance-faster-string-find.

># HG changeset patch
># User Jan Keromnes <[e-mail]>
>
>Bug 1468811 - Re-enable clang-tidy check performance-faster-string-find. r=sylvestre
>
>diff --git a/tools/clang-tidy/config.yaml b/tools/clang-tidy/config.yaml
>index d676d1d7d920..e61aa8648f86 100644
>--- a/tools/clang-tidy/config.yaml
>+++ b/tools/clang-tidy/config.yaml
>@@ -12,16 +12,17 @@ clang_checkers:
>   - name: -*
>     publish: !!bool no
>   - name: clang-analyzer-deadcode.DeadStores
>     publish: !!bool yes
>   - name: clang-analyzer-security.FloatLoopCounter
>     publish: !!bool yes
>   - name: clang-analyzer-security.insecureAPI.getpw
>     publish: !!bool yes
>+  # We don't add clang-analyzer-security.insecureAPI.gets here; it's deprecated.
>   - name: clang-analyzer-security.insecureAPI.mkstemp
>     publish: !!bool yes
>   - name: clang-analyzer-security.insecureAPI.mktemp
>     publish: !!bool yes
>   - name: clang-analyzer-security.insecureAPI.rand
>     publish: !!bool no
>   - name: clang-analyzer-security.insecureAPI.strcpy
>     publish: !!bool no
>@@ -67,18 +68,23 @@ clang_checkers:
>     publish: !!bool yes
>   - name: modernize-use-nullptr
>     publish: !!bool yes
>   - name: modernize-use-override
>     # Too noisy because of the way how we implement NS_IMETHOD. See Bug 1420366.
>     publish: !!bool no
>   - name: mozilla-*
>     publish: !!bool yes
>+  - name: performance-faster-string-find
>+    publish: !!bool yes
>   - name: performance-for-range-copy
>     publish: !!bool yes
>+# Only available from clang tidy 6.0. We are currently using 5.0
>+# - name: performance-implicit-conversion-in-loop
>+#   publish: !!bool yes
>   - name: performance-inefficient-string-concatenation
>     publish: !!bool yes
>   - name: performance-inefficient-vector-operation
>     publish: !!bool yes
>   - name: performance-type-promotion-in-math-fn
>     publish: !!bool yes
>   - name: performance-unnecessary-copy-initialization
>     publish: !!bool yes
>diff --git a/tools/clang-tidy/test/performance-implicit-conversion-in-loop.cpp b/tools/clang-tidy/test/performance-implicit-conversion-in-loop.cpp
>new file mode 100644
>index 000000000000..a7204380edd1
>--- /dev/null
>+++ b/tools/clang-tidy/test/performance-implicit-conversion-in-loop.cpp
>@@ -0,0 +1,9 @@
>+#include <map>
>+#include <string>
>+#include <utility>
>+#include <vector>
>+
>+int main() {
>+  std::map<int, std::vector<std::string> > my_map;
>+  for (const std::pair<int, std::vector<std::string> >& p : my_map) {}
>+}
>diff --git a/tools/clang-tidy/test/structures.h b/tools/clang-tidy/test/structures.h
>index 0225ac0c1b36..4f6fbb9ea1fd 100644
>--- a/tools/clang-tidy/test/structures.h
>+++ b/tools/clang-tidy/test/structures.h
>@@ -1,9 +1,9 @@
>-// Proxy file in order to define generic data types, to void binding with system headers
>+// Proxy file in order to define generic data types, to avoid binding with system headers
> 
> namespace std {
> 
> typedef unsigned long size_t;
> 
> template <class T>
> class vector {
>  public:
>
I wouldn't add this to our code since the patch is incomplete. It doesn't have the result of the test for 'performance-implicit-conversion-in-loop'. The entire goal of this kind of tests is to prevent having incomplete patches, and in fact this is what we are doing here.

Comment 15

[Jan Keromnes [:janx]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
>> Re-enable clang-tidy check performance-faster-string-find.
> 
> Could you please update the title before you commit?
> The patch is doing more than just "faster string"

Sure, will do. Thanks!

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #14)
>> Re-enable clang-tidy check performance-faster-string-find.
> 
> I wouldn't add this to our code since the patch is incomplete. It doesn't
> have the result of the test for 'performance-implicit-conversion-in-loop'.
> The entire goal of this kind of tests is to prevent having incomplete
> patches, and in fact this is what we are doing here.

I added that file for convenience, because when we upgrade to clang-tidy 6.0 we'll want to enable that test (and also run `./mach static-analysis autotest -d` to dump the expected result, otherwise the test would fail).

Do you really want me to remove this not-yet-useful-but-soon file? I think it's harmless (also, note that we've had performance-faster-string-find.cpp in tree for a while even though the checker wasn't enabled).

Comment 16

[Jan Keromnes [:janx]]

Attachment: Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml.

Rebased, r+ carried over.

Comment 17

[Jan Keromnes [:janx]]

Attachment: Fail static-analysis autotest when a clang-tidy check doesn't exist.

Comment 18

[Jan Keromnes [:janx]]

Attachment: Re-enable clang-tidy check performance-faster-string-find.

Addressed nits (mention drive-by fixes in commit message, remove unused test file), rebased, r+ carried over.

Comment 19

[Andi [:andi]]

As per our irc discussion I'm clearing this need info flag. 
Jan thank you!

Comment 20

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc44c3191f64f04dced973b3e4de78bb9d9d8d9

Two problems:

1) `Sa` job fails with:

> /builds/worker/workspace/build/src/tools/clang-tidy/test/performance-faster-string-find.cpp:1:10: error: 'string' file not found [clang-diagnostic-error]
> Return code: 6
> 'mach static-analysis autotest --intree-tool' did not run successfully. Please check log for errors.

2) `yaml` job warns with:

> TEST-UNEXPECTED-WARNING | tools/clang-tidy/config.yaml:80:1 | comment not indented like content (comments-indentation)

Comment 21

[Andi [:andi]]

Please see this how it's indented: https://dxr.mozilla.org/mozilla-central/source/tools/clang-tidy/config.yaml?q=clang-tidy%2Fconfig.yaml&redirect_type=direct#15

Comment 22

[Jan Keromnes [:janx]]

Attachment: Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml.

Rebased, carried over.

Comment 23

[Jan Keromnes [:janx]]

Attachment: Fail static-analysis autotest when a clang-tidy check doesn't exist.

Rebased, carried over.

Comment 24

[Jan Keromnes [:janx]]

Attachment: Re-enable clang-tidy check performance-faster-string-find.

- Rebased
- Fixed `yaml` lint by properly indenting a comment
- Fixed `Sa` by using an internal mock std::string::find instead of depending on system headers
- R+ carried over

Comment 25

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f95ab15b1dea43f04e9fe8c5863db16525d0b01

Comment 26

[Jan Keromnes [:janx]]

Green!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32fbc8568c71
Sort clang-tidy checks alphabetically in tools/clang-tidy/config.yaml. r=sylvestre
https://hg.mozilla.org/integration/mozilla-inbound/rev/e276f2617d4e
Fail static-analysis autotest when a clang-tidy check doesn't exist. r=sylvestre
https://hg.mozilla.org/integration/mozilla-inbound/rev/50e3ae4528fb
Re-enable clang-tidy check performance-faster-string-find. r=sylvestre

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/32fbc8568c71
https://hg.mozilla.org/mozilla-central/rev/e276f2617d4e
https://hg.mozilla.org/mozilla-central/rev/50e3ae4528fb