Adding Static Analysis and other checks to clang-tidy

[kuzminrobin]

No description provided.

[kuzminrobin]

FYI: These checks in .clang-tidy file are added to the default ones: clang-diagnostic-*,clang-analyzer-*.
The clang-diagnostic-* enables the warnings of the Clang compiler.
The clang-analyzer-* enables the static analyzer. I.e. the Clang Static Analyzer is enabled by default, but our .clang-tidy file was disabling all the checks with the fragment -*. I removed -*, which enabled the Clang Static Analyzer.

[troels-im]

Overall looks good to me. I think there is a question to how closely we want to align the style between the passes and the runtime, but that is not necessarily something we need to deal with in this PR.

[troels-im]

Is it worthwhile that we use same style for variable names? In the passes code, we use snake case for variables.

[kuzminrobin]

I believe everybody would benefit from the same coding style in QIR RT and Passes. I personally don't have particular preference. I'm fine with camelBack or snake_case or other.
Added #817.

[kuzminrobin]

Please review. Troels' review is not sufficient. Approval required by reviewers with write access.

[swernli]

Looks good, one minor comment but not really important so I leave it up to you if you want to address it.

[swernli]

nit: I'm not sure I like this pattern, though honestly there isn't anything functionally wrong here. It just seems odd that this assert message would show up only if the reference count was updated with a decrement value less than -1, where if the decrement is exactly -1 (our compiler's QIR default) on an item of 0 references then the assert from within Release will fire instead. That assert already protected us from decrementing below zero, so this new one is arguably extraneous, though not harmful.

[kuzminrobin]

I'm totally with you on our scenario.

But looks like the static analyzer has considered a different scenario and complained for a possible released (dangling) pointer dereference. I'll try to tell what I remember.

Let's consider a callable with callable->refCount equal 1. And someone calls __quantum__rt__callable_update_reference_count() with increment parameter -2. During the first most iteration of this for loop (i == -2) the callable->Release() is invoked. It decrements the callable->refCount to 0, deallocates the callable, and returns 0. After the first call to callable->Release() the callable becomes a dangling pointer.

Before this change, the value 0 returned by callable->Release() was not analyzed, and another iteration of the loop was starting. At the moment of the second call callable->Release() a dangling pointer was accessed which immediately resulted in the undefined behavior (UB), before the execution reached an assert inside of QirCallable::Release().

I agreed with the complaint from the static analyzer and tried to prevent the UB with an assert (in green line 107). I don't state that my change is the best fix (e.g. the break in the green line 108 can be redundant ;-) ), feel free to propose a better fix.

[swernli]

Ah, that's a great point! I had incorrectly assumed Release would clean up internal structures but not invalidate the callable pointer itself, but looking at the implementation that is not the case. Score one for the analyzer ;)