- User Since
- Sep 17 2012, 3:16 AM (331 w, 2 h)
To add an analogy, Clang Tidy will not require C++ Core Guidelines related checks to be evaluated on projects that are not following the guidelines as the results are meaningless for those projects.
Thu, Jan 17
Any objections to commit this?
I think this is quiet coding guideline specific check which is useful for a set of security critical projects. As this is an opt in kind of check, I think it does no harm to have it upstream.
Wed, Jan 16
Thanks, LGTM! It is interesting to see if we need to traverse all the super regions in scanReachableSymbols, but if we need to change something there, I would prefer that to be in a separate patch.
If visiting the whole super region chain proved to be redundant I would recommend removing it for clarity regardless of having a performance impact.
Mon, Jan 14
I really like all this detective work and it would be sad to have it forgotten. I would love to see some of your comments in the documentation of symbol reaper.
Tue, Jan 8
Some nits inline. Otherwise looks good to me.
Dec 18 2018
Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.
Dec 14 2018
Sorry for the delay. The changes are looking good to me, although I think it might be worth to split this up into two patch, one NFC with the renaming, and one that actually introduces the changes.
Dec 7 2018
While Static Analyzer is the only client of CTU library at the moment, we might have more in the future. I would not use the phrase ANALYZE in the log message. Once this is resolved the rest looks good.
Dec 6 2018
Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.
Dec 5 2018
Dec 4 2018
Having an analyzer config option makes sense.
The code LGTM! I am not good at wordsmithing, but if the descriptions of the statistics are not clear enough, I agree that they should be rephrased.
After the review comment is resolved, the rest LGTM!
Dec 3 2018
- Addressed further comments.
Nov 27 2018
Overall looks good to me, some minor comments inline.
Nov 23 2018
Some minor comment inline. Otherwise looks good.
Nov 17 2018
Nov 15 2018
It would be great to have a way to extend the list of (possibly non-stl) types to check. But I do understand that the analyzer does not have a great way to set such configuration options right now.
Looks good. Do we plan to detect problems other than use after move? Maybe it would be worth to synchronize with the tidy checker name use-after-move or is it going to cause more confusion?
Nov 12 2018
I do like the idea of moving the Clang Static Analyzer documentation to where the rest of the tools are documented. I believe the original reason the analyzer had a separate homepage is due to it was off by default in clang at the beginning and users downloaded it from the separate page.
- Use the term checker instead of check.
Nov 10 2018
- Move the checklist up before additional info in the HTML file.
- Fix minor nits.
- Add missing bullet points (thanks @Szelethus for noticing)
Nov 6 2018
I would love to see a test with deeper macro in macro expansion and larger number of arguments, with some of the arguments unused. Some minor nits inline, otherwise looks good.
Nov 2 2018
This new version based on the bullets by NoQ. I also included some additional ones from other lists and added some new ones, e.g. the NamedDecl::getName will fail if the name of the decl is not a single token. I also reordered a bit. Advice that is more advanced and guidelines that are less likely to be violated should be closer to the bottom of the list.
LGTM, but let's wait for @NoQ before committing.
I also would like to see in a tool how this would look like as an event before committing :) Just a sanity check to make sure this feature makes sense. Could you post a screenshot of CodeChecker or any other tool using this feature?
One question otherwise looks good.
One question and one nit otherwise looks good. Feel free to commit once those are resolved without another round of reviews.
Nov 1 2018
- Remove yet another dependency from the tool that is no longer used.
Oct 31 2018
Please add a test case where a bug path goes through a macro definition and this macro is undefed at the end of the translation unit.
LGTM! Thanks, I think it is much easier to understand what is going on this way.
Oct 30 2018
I only wonder if this should be on by default or guarded by a config option. I do not have strong feelings about any of the options though.
Oct 29 2018
Oct 22 2018
I agree with NoQ. Forward and backward compatibility might be important for CodeChecker as well.
But I wonder if it make sense to have analyzer-config compatibility mode on a per config basis?
E.g., if we have two configs:
- One did not exist in earlier clang versions, but a tool (like CodeChecker) would like to pass this to the analyzer without doing a version check first. Passing this in a compatibility mode makes sense. This could be the regural -analyzer-config
- The second option also did not exist in earlier clang versions, but we do not want to support those versions. In the case passing this config in a more strict mode makes sense. This could be something like -analyzer-config-strict.
Overall looks good, minor comments inline.
Overall looks good if the community agrees with the directions. Some comments inline.
Oct 18 2018
Oct 10 2018
I am not sure what to do about implcit checks. Those are probably should never be turned on or off by the user, but they should be on or off by default based on the set of checks the user enabled and the platform she is using. Thus, I am perfectly ok with the implicit_checks.html only being accessible from the checker development manual. Maybe we should extend the checker list with a notice that the user should never disable the core checks.
Oct 9 2018
Oct 8 2018
- Added the ideas from Kristof.
I agree that it would make sense to either not have arguments that are not represented in the AST or create expressions for those implicit arguments.
Sep 6 2018
I like that we print the program points for hidden nodes :) Great work!
Aug 21 2018
Oh, and thanks for working on this, this improvement was long overdue, but everybody was busy with something else :)
Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.
Aug 15 2018
Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.
Aug 2 2018
We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple.
But consider this as an optional task if you have nothing better to do. I am ok with committing this as is.
Jul 27 2018
Jul 25 2018
Small comments inline.
Jul 23 2018
Some comments, mostly nits inline.
Jul 19 2018
Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks.
Jul 11 2018
Jul 9 2018
Don't you need to edit the tests as well?
Jul 8 2018
Thanks! The changes look good, I forgot to mark one double lookup though in my previous review.
Overall looks good, some nits inline. Let's run it on some projects to exercise this change.
Jul 4 2018
Jun 26 2018
Ok, it looks like naively just dropping all the constraints at Z3 is not the most efficient way.
Jun 25 2018
Jun 24 2018
LGTM, given that the assert does not fire for the projects you tested on.
Jun 23 2018
Looks better, thanks!
Regarding the visitor:
Maybe rather than looking at the AST, we should check the states, when we started to track the returned symbol?
Jun 18 2018
I wonder if this could be done when plist is emitted generally, independent of any checks.
Jun 13 2018
Jun 11 2018
Having C++ support would be awesome!
Thanks for working on this!
Jun 10 2018
Just for the record, there is a common example where relying on copy elision might bite and google do not recommend relying on it for correctness: https://abseil.io/tips/120
Jun 5 2018
Jun 4 2018
So having an analyzer-config option would be useful for those codebases that are expected to be compiled with less advanced compilers that do not elide copies or not doing it aggressively enough. Maybe it is worth to ask on the mailing list of the community wants to have an option for this? I am not sure though how often the end users end up fine tuning the analyzer. It might be nice to have a guide how to do that (and in what circumstances does each of the config values make sense).
Jun 1 2018
Sorry for the limited activity. Unfortunately, I have very little time reviewing patches lately.
I think we need to answer the following questions:
- Does this change affect the analysis of the constructors of global objects? If so, how?
- Do we want to import non-const object's initializer expressions? The analyzer might not end up using the value anyways.
- How big can the index get with this modification for large projects?