- User Since
- Aug 23 2013, 11:08 AM (221 w, 1 d)
Mon, Nov 13
Tue, Oct 31
Also if you check the code snippets in the coding standard: https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto you can see that where we officially put the references.
Mon, Oct 30
I'd also include some info on how it's now possible to dump the issue hash. You introduce a new debugging function here "clang_analyzer_hashDump" but it's not mentioned in the commit message.
Fri, Oct 27
Thanks! Do you have commit access or should we commit on your behalf?
What kind of output will this start displaying?
Please, change the commit description to be more comprehensive.
Just to be clear, since this leads to regression to the checker API, I am asking to look into other ways of solving this problem. For example, is there a way to ensure that the checker names are set at construction?
Mon, Oct 23
Sat, Oct 21
Fix looks good. Could you add a test case? The analyzer tests define their own stdin and types, so you could add an alternate definition or change the existing one so that it goes through a typedef.
Fri, Oct 20
Thu, Oct 19
Oct 13 2017
Looks like the need to have the checker name in BugType along with the checker names not being initialized early enough, leads to worse checker-writer experience. Is there a way to ensure that the checker names are set at construction?
Oct 10 2017
Once the comments by @paquette are addressed, LGTM. Thanks!
Sep 29 2017
Sep 28 2017
Sep 25 2017
Sorry for the wait!
Looks great! Thanks!
Sep 18 2017
Please, commit. I think we can take further comments post-commit.
Sep 15 2017
Looks like I rushed into accepting. This needs a test case of cause!
Sep 14 2017
Sep 13 2017
How about committing the refactor of the code without test modifications. And committing changes to the test separately?
Sep 6 2017
Thanks for addressing the non-determinism!!!
Yes, please, commit.
Aug 28 2017
But I've never used the taint tracking mode, so I don't know what would be a reasonable default for MaxComp.
that one is very experimental anyway. I'd just keep the functional changes to tain out of this patch and use the current default that taint uses.
Is this blocked on the same reasons as what was raised in https://reviews.llvm.org/D35109?
Aug 27 2017
Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.
I think we should have these is .rst format as this is what the rest of the documentation predominantly uses. (Note, the formatting can be very basic, it's the format that I care about.)
Aug 25 2017
Aug 20 2017
Aug 10 2017
Aug 9 2017
What do you suggest? Should we widen the type of the difference, or abandon this patch and revert back to the local solution I originally used in the iterator checker?
Aug 1 2017
I tried to keep this as a minimal starting example because this currently blocks @yamaguchi 's GSoC project for bash completion. There we want to complete the values for -analyzer-config and we currently don't have a good way to get a complete list of available configs from the driver :).
Jul 31 2017
Jul 20 2017
Jun 28 2017
I agree that we should not print the values of all variables. The users will be overwhelmed with the huge amount of information. It is very valuable to highlight just the right information. (I believe even the current diagnostics often produce too much info and highlighting the more important notes would be very valuable.) However, the examples you presented are very compelling. Are there ways we could highlight the same information without printing values of all variables?
Jun 27 2017
Jun 26 2017
@dcoughlin As a reviewer of both patches - could you tell us what's the difference between them? And how are we going to resolve this issue?
Jun 22 2017
This just supports the statement that this particular check should not go under unix. I understand that it will be inconsistent with the name of the malloc checker, which we probably should not change as people might be relying on the package names. I think it's better to have inconsistency than having checks applicable to windows in a package named portability.unix. If there will be checks that need to be in portability and only used for unix, we could create that sub-package later on.
Jun 20 2017
Hmm, should there be new tests that demonstrate that we cover the new APIs?
Unless we use a new registration mechanism for some of these APIs, I'd be fine without adding a test for every API that has localization constraints.
The problem here is not in ScopeContext - it isn't going to be a huge patch. The problem is CFG-related mostly - there are many corner cases. We'll also need some help with
fixing Objective-C checkers (RetainCount, for example).
In particular, there are patches to generate more symbolic expressions, that could also degrade the performance (but fix some fixmes along the way).
Jun 17 2017
Jun 16 2017
LGTM. Thank you!
eg. checkers for portability across linux/bsd should be off on windows by default, checkers for non-portable C++ APIs should be off in plain C code, etc
Is the checker you are moving to portability off and not useful on Windows?
Thanks! Do you have commit access?
Once Artem gives more details about the codebase he tested on, I think it would be fine to commit this. We can revert/address concerns later if @a.sidorin or anyone else raises concerns based on further testing on their codebases. @a.sidorin, would this work for you?
Maybe we should introduce another UMK_* for deeper analysis instead?
The difference here is not substantial enough to warrant a new level. The general motivation for bumping these numbers is that we've set the timeouts many years ago and the hardware improved quite a bit since then.
Jun 15 2017
-(Anna) Scan-build-py integration of the functionality is nearly finished (see https://github.com/rizsotto/scan-build/issues/83) (--ctu switch performs both analysis phases at once). This I think could go in a different patch, but until we could keep the ctu-build.py and ctu-analyze.py scripts. Do you agree?
Jun 14 2017
Looks good overall, but we should try and extend isCalled if possible.
Should this revision be "abandoned" or is the plan to iterate on it?
Looks good with a nit!
Great cleanup! Some comments below.
This generally sounds good. Definitely do submit these changes in small pieces! See http://llvm.org/docs/DeveloperPolicy.html#incremental-development for rationale.
Jun 12 2017
Jun 5 2017
Jun 1 2017
These are great additions!
May 31 2017
May 23 2017
LGTM. Thank you!
May 22 2017
Is it possible to add a regression test that demonstrates the IR pattern this diff is fixing?
May 17 2017
Thanks! A couple of minor comments below.
May 16 2017
Looks good. Thank you!
Do you have commit access or should we commit?
May 10 2017
Thank you for the patch! Could you please re-submit the patch with context? Instructions on how to do that can be found here:
May 9 2017
That wouldn't work this way because we'd have the completely redundant "calling property accessor" piece before that, and "returning..." after that.
Sorry for the delay!!!
I am fine to land this without a test. @szepet, Do you have commit access or should we commit on your behalf?
These new "extra notes" of mine might be useful (we could put them at property declaration), i could add them if everybody likes this idea.
What are these? Is there a PR?