- User Since
- Apr 6 2020, 5:32 AM (18 w, 2 d)
Awesome, thanks! Buuuut, maybe we can use \ref form, it looks like it's a preferred form in the codebase.
Fri, Aug 7
Thu, Aug 6
It is an interesting checker, but it seems that this kind of checker is extremely hard in single TU/top-down analysis. It feels like it's going to produce hell a lot of false positives in the wild.
Also mutex is usually a global variable - the nemesis of any static analysis tool.
Keep SATest.py Python2 compatible
Wed, Aug 5
Hey, thanks again for cleaning up the analyzer's docs 😄
Great job, I'm on board with this change!
Other than parameter names, it looks totally reasonable to me.
Tue, Aug 4
Mon, Aug 3
Ready! Sorry, I remember, you've already told me.
Fri, Jul 31
Hey, nice catch!
However, I'm going to complain about commit messages again 😅 I would prefer having imperative mood in the message, something like "Refactor ..." or "Introduce minor refactoring..."
Thanks for working on improving the quality of the codebase!
I again have to nitpick about the commit message, can you please change it to "Simplify ..."?
Thu, Jul 30
Other then the naming issue, I don't see any problems with this change!
LGTM! Thanks for all the investigative work!
Wed, Jul 29
LGTM! Thanks for the fast fix!
Tue, Jul 28
Thu, Jul 23
LGTM, thanks for taking care of this!
Wed, Jul 22
Make --projects and --max-size compatible
Mon, Jul 20
Thu, Jul 16
The same thing with the commit message here, it would be better as just "Change"
It is a good practice in many projects to make commit messages into imperative (i.e. "Improve" instead of "Improving" or "Improved"). Again, not everyone follows it, but it is good to keep it consistent, right?
Additionally, I would prefer commit message to be imperative. It is sorta like a rule of a good commit message.
Wed, Jul 15
Tue, Jul 14
@xazax.hun You were interested in performance ⏫
Maybe spend a few minutes remeasuring libsoundio more carefully, just in case?
Sure, will do!
I do like this change even apart from the clang-tidy integration epic. I strongly believe that decoupling (when done right) makes things easier and this change seems like a good first step.
Mon, Jul 13
Nice work! I like the functionality change of it, but I'm a bit worried about the category change. When we move a checker out of alpha, I would prefer to have more information and numbers on how this checker performs on a good variety of projects.
If you ask me, the numbers I'm the most interested in are the number of warnings on different projects and how this number corresponds to the overall number of warnings that the analyser emits for all non-alpha checkers together and individually. It will help us to see how noisy the new checker is (it is not a lint check, it should be pretty rare), and see if it belongs in non-alpha.
Jul 13 2020
@OikawaKirie One last thing, what's your preferred name and email for the commit? Thanks!
Should I continue waiting for other reviewers?
I have some test results with execution times. Because the execution time varies from one execution to another, I've measured performance on master twice (I know that it's not very conclusive) to show that equivalence tracking doesn't affect performance of the analyzer. So, please, let's treat it more like a smoke test that everything stayed the same with a new functionality rather than a full-blown performance benchmarking.
Jul 10 2020
Here is an example of how benchmarking chart looks like:
Add matplotlib and graphviz to requirements.txt
Jul 9 2020
Fix review remarks
I thought so too, but it looks like glob works in mysterious ways
Jul 8 2020
I think that the problem is actually with the second '/', but I decided to retreat back to the form consistent to the rest glob.glob invocations in this file
Hi @ASDenysPetrov no problem at all! Thanks for taking your time and checking it.
Jul 7 2020
Get rid of 'isZero'
Fix comments and add a test for downcasts
Jul 6 2020
Fix review remarks