[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers

2017-07-11 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Thanks for the patch. It is really great to see these documented! Who is the target of this documentation? Is it developers of the analyzer or is it end users of the analyzer? If it is end users, it is unfortunate that we've been just grabbing examples from the regres

[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers

2017-07-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Do you have commit access, or do you need someone to commit it for you? https://reviews.llvm.org/D33645 ___ cfe-commits mail

[PATCH] D28954: [analyzer] Add support for symbolic float expressions

2017-07-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Thanks for the patch, and apologies for the delay reviewing! Here are some initial comments -- there are more coming. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:159 +llvm::APFloat To(S, llvm::APFloat::uniniti

[PATCH] D28954: [analyzer] Add support for symbolic float expressions

2017-07-15 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. This looks like a very solid start! One area that I'm very worried about is that in various places the analyzer makes assumptions about algebraic simplifications that don't hold for floating point because of NaN and other floating point oddities. I think it really im

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-17 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Looks good to me, other than a nit on indentation and a request for a little bit more info in a comment! Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3394 + // See if the function has 'rc_ownership_trusted_implementation' + // ann

[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers

2017-07-17 Thread Devin Coughlin via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL308242: [analyzer] Add missing documentation for static analyzer checkers (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D33645?vs=106184&id=106989#toc Repository: rL LLVM

[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers

2017-07-17 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Committed in r308242. Thanks Dominik! Repository: rL LLVM https://reviews.llvm.org/D33645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-17 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3412 + if (FD->getDefinition()) { +canEval = hasRCAnnotation(FD->getDefinition(), + "rc_ownership_trusted_implementation"); --

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-17 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. > So, should I submit another patch on the same revision after modifying the > title, summary, etc. or should I create another revision for that? You should create another revision. You can mark it as dependent on this one in Phabricator. Repository: rL LLVM http

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-18 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! One thing I should have noted in a prior review is that the helper functions should be declared 'static' so that we don't have a public symbol for them. Making them static will pre

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-18 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Committed in r308416. Repository: rL LLVM https://reviews.llvm.org/D34937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

2017-07-18 Thread Devin Coughlin via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL308416: [analyzer] Add annotation attribute to trust retain count implementation (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D34937?vs=107020&id=107243#toc Repository:

[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.

2017-07-18 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. In this case, I would be fine with some sort of AbstractStorageMemoryRegion that meant "here is a memory region and somewhere reachable from here exists another region of type T". Or even multiple regions with different identifiers. This wouldn't specify how the memor

[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker

2017-07-19 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. This looks pretty good. There are some minor comments in line. One thing that is missing: tests for the new diagnostic text. Can you add new tests that specifically test for the new diagnostic text with // expected-warning {{ ... }}}. These should probably go in retai

[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker

2017-07-25 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I will commit. Repository: rL LLVM https://reviews.llvm.org/D35613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker

2017-07-25 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Sorry for the delay! This looks good to me. We have a really embarrassing FIXMELATER from 2012 (!!!) that disabled the plist tests for diagnostics. This means we're not getting testing f

[PATCH] D35674: [analyzer] Treat C++ throw as sink during CFG-based suppress-on-sink.

2017-07-25 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Do we have a radar for this? It sounds familiar. Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp:3313 +static bool isNoReturnBlock(const CFGBlock *Blk) { + if (Blk->hasNoReturnElement()) Maybe a better name for this w

[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker

2017-07-25 Thread Devin Coughlin via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL308990: [analyzer] Add diagnostic text for generalized refcount annotations. (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D35613?vs=107471&id=108115#toc Repository: rL L

[PATCH] D35673: [analyzer] A better CFG-based suppress-on-sink.

2017-07-25 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp:3313 +static bool isDominatedByNoReturnBlocks(const ExplodedNode *N) { + const CFG &Cfg = N->getCFG(); Do you really mean "is dominated by"? That is, "every path fro

[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

2017-07-26 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I have some concerns about soundness when the rearrangement may overflow. Here is an example: void clang_analyzer_eval(int); void foo(signed char A, signed char B) { if (A + 0 >= B + 0) { clang_analyzer_eval(A - 126 == B + 3); // This yields FALSE with

[PATCH] D41935: [analyzer] NFC: Mark default constructors for ProgramPoints.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang https://reviews.llvm.org/D41935 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D40560: [analyzer] Get construction into `operator new` running in simple cases.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM with the TODO and the test case I requested inline. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:487 +if (const MemRegion *MR = I.second.getAsRegion()) +

[PATCH] D41250: [analyzer] Model implied cast around operator new().

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This looks good to me, as well. https://reviews.llvm.org/D41250 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. LGTM as well. https://reviews.llvm.org/D41266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41934: [analyzer] Fix CXXNewExpr callback order.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks great. It is nice to have this fixed and cleaned up! Comment at: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp:95 + llvm::errs() << "PreCall"; +

[PATCH] D41797: [analyzer] Suppress escape of this-pointer during construction.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang https://reviews.llvm.org/D41797 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D41881: [analyzer] Flag bcmp, bcopy and bzero as obsolete

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks for adding these! This looks good to me. Do you have commit access, or do you need someone to commit this? Repository: rC Clang https://reviews.llvm.org/D41881

[PATCH] D42015: [analyzer] NFC: RetainCount: Don't dump() regions to the user.

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Thanks for fixing this. Repository: rC Clang https://reviews.llvm.org/D42015 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D41816: [analyzer] Model and check unrepresentable left shifts

2018-01-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. The diagnostic text looks to me, but I do have a comment about the nested 'if' inline. Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:150 +SB.getKnownValue(state, C.getSVal(B->getRHS())); +if ((unsigned) RHS->getZE

[PATCH] D42266: [analyzer] Prevent AnalyzerStatsChecker from crash

2018-01-21 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. This seems reasonable. Would it make sense to use the last element of the block edge's source for the diagnostic location when the destination block is empty? Repository: rC Clang https://reviews.llvm.org/D42266 ___ c

[PATCH] D41816: [analyzer] Model and check unrepresentable left shifts

2018-01-21 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. Looks good to me, thanks! https://reviews.llvm.org/D41816 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

2018-01-21 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517 +if (const auto *Array = dyn_cast(DeclRef->getType())) { + uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8; + if (const auto *String = dyn

[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.

2018-01-31 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. However, I do think you should take George's suggestion to have makeZeroElementRegion() have a boolean out parameter rather than a EvalCallOptions out parameter. T

[PATCH] D42672: [CFG] [analyzer] Heavier CFGConstructor elements.

2018-01-31 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I think it will be great to have a more explicit representation of construction in the CFG! Comments in line. Comment at: include/clang/Analysis/CFG.h:143 + CXXConstructExpr *Constructor; + Stmt *Trigger; + I think it would be good

[PATCH] D42672: [CFG] [analyzer] Heavier CFGConstructor elements.

2018-02-02 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me, although as I noted inline I think both the name and the comment for VisitForConstructionContext() are confusing. If you can be more precise I think it would help

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

2017-02-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Thanks for updating the tests to be able to run both the z3 and range-constraint managers! I think this will make it much easier to ensure the z3-backed manager continues to work as as expected moving forward. I suggested an alternative approach inline to running the

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

2017-02-24 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Dominic, this (https://reviews.llvm.org/D28952) and https://reviews.llvm.org/D26061 look get to me! Let's get these two committed! We'd like to get to a place where in-tree incremental

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-02-26 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D30373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-02-28 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Saving the relevant portion of the build bot log output before it disappears from the bot: 0.000 [0/1/0] Running all regression tests -- Testing: 31894 tests, 80 threads -- Traceback (most recent call last): File "", line 1, in File "C:\Python27\lib\mult

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-02-28 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I'm not sure what is going on. One thing to try is defining the class in clang/test/lit.cfg -- which seems to work for Swift. (See https://github.com/apple/swift/blob/master/test/lit.cfg#L137) We'd still want to only use that class in Analysis/lit.local.cfg. Another

[PATCH] D30499: [analyzer] pr32088: Don't destroy the temporary if its initializer causes return.

2017-03-01 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Seems like a safe quick fix for the crash. Looks good to me! https://reviews.llvm.org/D30499 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D30499: [analyzer] pr32088: Don't destroy the temporary if its initializer causes return.

2017-03-01 Thread Devin Coughlin via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL296646: [analyzer] pr32088: Don't destroy the temporary if its initializer causes… (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D30499?vs=90178&id=90201#toc Repository:

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-03-02 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. In https://reviews.llvm.org/D30373#689613, @ddcc wrote: > With the first method, I'm not sure how to refer to the `AnalyzerTest` class > defined in `lit.cfg` from `lit.local.cfg`. It doesn't seem to be in scope, so > unless I store an instantiation in the `config` obj

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-03-02 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Thanks for sticking with this!! This looks good to me, although I would prefer if the definition of the AnalyzerTest class were moved to the bottom of the lit.cfg file. The other configuration and substitutions are far more important to the rest of clang -- people sh

[PATCH] D30373: [analyzer] NFC: Update test infrastructure to support multiple constraint managers

2017-03-03 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Looks great. Thanks!! https://reviews.llvm.org/D30373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30565: [analyzer] Terminate analysis on OpenMP code instead of crashing

2017-03-03 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks for fixing this! It looks like this is tracked by PR31835. https://bugs.llvm.org//show_bug.cgi?id=31835 https://reviews.llvm.org/D30565 ___

[PATCH] D30565: [analyzer] Terminate analysis on OpenMP code instead of crashing

2017-03-03 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. @zaks.anna: What do you think? Should we try to get this into clang 4.0? Repository: rL LLVM https://reviews.llvm.org/D30565 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-03 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added reviewers: zaks.anna, NoQ. dcoughlin added a comment. The analyzer has two different kinds of diagnostics: AST-based and path-sensitive. AST-based diagnostics are similar to the diagnostics that clang performs in Sema in that they can usually be localized to a single program poi

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. We add the suppression for libcxx issues in LikelyFalsePositiveSuppressionBRVisitor in BugReporterVisitors.cpp. It is ad-hoc pattern recognition of idioms we know cause false positives rather than an explicit database. Repository: rL LLVM https://reviews.llvm.org

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-07 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Kevin, would you be willing to file a PR on https://bugs.llvm.org with the 9 false positives you are seeing? This will help us suppress them for users who don't use 'suppress-c++-stdlib'. Repository: rL LLVM https://reviews.llvm.org/D30593 _

[PATCH] D30798: [analyzer] Turn suppress-c++-stdlib on by default

2017-03-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM, https://reviews.llvm.org/D30798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members

2016-11-28 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This is great! Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:205 - // We need to be careful about treating a derived type's value as - // bindings for a base t

[PATCH] D25475: [analyzer] Add a new SVal to support pointer-to-member operations.

2016-11-30 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. PointerToMemberData looks like it is on the right track! One thing that is still missing is using the base specifier list to figure out the correct subobject field to read and write from. This is important when there is non-virtual multiple inheritance, as there will

[PATCH] D22862: [analyzer] Fix for PR15623: eliminate unwanted ProgramState checker data propagation.

2016-11-30 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. Overall, this looks good to me. Thanks for tackling this! One request, though. Could you move the tests into already existing test files? We're trying to avoid test files that only test a single issue. This makes it easier for people

[PATCH] D26691: [analyzer] Run clang-format and fix style

2016-12-01 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. In https://reviews.llvm.org/D26691#610292, @zaks.anna wrote: > I am not a big fan of loosing svn blame only to fix formatting, but since you > are modifying this code anyway, it's fine by me. > > Artem and Devin, what is your opinion on this? I agree that in general

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha

2016-12-01 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin updated this revision to Diff 79981. dcoughlin added a comment. - Update the diagnostic to be explicit about whether the issue occurs during construction or destruction - Add test for pure, intraprocedural case (Sema also catches this). https://reviews.llvm.org/D26768 Files: includ

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha

2016-12-01 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin updated this revision to Diff 79992. dcoughlin added a comment. - Add a PureOnly analyzer-config option that, when set, will limit diagnostics to calls to only pure virtual functions. This should have gone in with the prevision updated diff; my apologies for the noise. https://revie

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha

2016-12-01 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin marked 2 inline comments as done. dcoughlin added a comment. In https://reviews.llvm.org/D26768#607157, @zaks.anna wrote: > Not sure if we should make pure vs not an option so that users could turn the > checking off. Is there a way to suppress the warning? There is not a way to supp

[PATCH] D27408: [analyzer] RetainCountChecker: remove unused enum value; NFC.

2016-12-05 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h:179 - static RetEffect MakeOwned(ObjKind o, bool isAllocated = false) { -return RetEffect(isAllocated ? OwnedAllocatedSymbol : OwnedSymbol, o); + static RetEffect MakeOw

[PATCH] D27409: [analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

2016-12-05 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Looks good to me! To ease the future maintenance burden I would suggest moving the test into 'retain-release-arc.m' unless it really needs to be in its own separate file. Comment at: test/Analysis/dispatch-data-leak.m:59 +#endif +}

[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. In https://reviews.llvm.org/D27409#614601, @NoQ wrote: > Not sure we need to stay merged with `retain-release-arc.m`, as we're not > really reusing many declarations across these files. I find it more helpful to organize tests around functionality ('retain count che

[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. The analyzer currently doesn't do any checking for dispatch retain/release APIs in C. It similarly doesn't do any checking in Objective-C when OS_OBJECT_USE_OBJC is 0 (and thus the dispatch types are defined to their C-struct versions). This happens when the user expl

[PATCH] D27409: [analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

2016-12-07 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Ship it! :-) https://reviews.llvm.org/D27409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-07 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. This looks great to me other than the idiom I mentioned inline (which is common, as I have found to my chagrin). Comment at: lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp:12 +// Currently finds only one kind of issue: +// - Find autosynthesiz

[PATCH] D13134: [analyzer] Add keyboard shortcuts to report.html

2016-12-07 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin commandeered this revision. dcoughlin edited reviewers, added: skomski; removed: dcoughlin. dcoughlin added a comment. Commandeering and closing this revision as it has been more than a year. Repository: rL LLVM https://reviews.llvm.org/D13134

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp:44 + BugReporter &BR) const { + if (D->isReadOnly() || D->getSetterKind() != ObjCPropertyDecl::Copy) +return; It is pro

[PATCH] D27600: [analyzer] Refine the diagnostics in the nullability checker to differentiate between nil and null

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I think in the 'null' case it might be better to keep it as "Null passed" or even "Null value passed". This is different than the 'nil' case because the diagnostic is not referring to a literal. https://reviews.llvm.org/D27600 _

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I evaluated this checker on three internal codebases that make large use of virtual functions. Project 1: ~190,000 lines of C++. 16 alarms. I triaged all of them. There were 2 definite false positives (FPs) and 14 likely FPs. Project 2: ~320,000 lines of C++. 116 alar

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin retitled this revision from "[analyzer] Improve VirtualCallChecker diagnostics and move out of alpha" to "[analyzer] Improve VirtualCallChecker diagnostics". dcoughlin updated this revision to Diff 80953. dcoughlin added a comment. Keep the diagnostic improvements but remove the change

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move to optin package.

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin retitled this revision from "[analyzer] Improve VirtualCallChecker diagnostics" to "[analyzer] Improve VirtualCallChecker diagnostics and move to optin package.". dcoughlin updated the summary for this revision. dcoughlin updated this revision to Diff 80979. dcoughlin added a comment.

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move to optin package.

2016-12-09 Thread Devin Coughlin via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL289309: [analyzer] Improve VirtualCallChecker diagnostics and move into optin package. (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D26768?vs=80979&id=80984#toc Repository

[PATCH] D26768: [analyzer] Improve VirtualCallChecker diagnostics and move to optin package.

2016-12-12 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. In https://reviews.llvm.org/D26768#619222, @malcolm.parsons wrote: > In https://reviews.llvm.org/D26768#618651, @dcoughlin wrote: > > > The definite false positives were cases where the programmer seemed aware > > of the semantics of virtual calls during construction/d

[PATCH] D27726: [analyzer] Refer to macro names in diagnostics for macros representing a literal

2016-12-13 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. The diagnostics look great to me and the macro logic seems reasonable. Comment at: test/Analysis/diagnostics/macros.cpp:49 +} \ No newline at end of file -

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

2017-04-20 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added inline comments. Comment at: include/clang/AST/Decl.h:53 class VarTemplateDecl; +class CompilerInstance; Is this needed? It seems like a layering violation. Comment at: include/clang/AST/Mangle.h:59 + // the static analyzer.

<    1   2   3