[PATCH] D55907: [analyzer] RetainCount: Bluntly suppress the CFRetain detection heuristic on a couple of CM functions.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me, but please have George take a look too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55907/new/ https://reviews.llvm.org/D55907

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

dcoughlin added a comment. I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet. Often they don't

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

dcoughlin added a comment. In https://reviews.llvm.org/D46159#1081371, @lebedev.ri wrote: > Note that it is completely off by default, and is not listed in > documentation, `clang-tidy --help`, > so one would have to know of the existence of that hidden flag first, and > only then when that fl

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

dcoughlin added a comment. In https://reviews.llvm.org/D46159#1085548, @pfultz2 wrote: > > Do you have some better choices? > > I could do `-allow-alpha-checks`. What do you think? That seems reasonable to me. Although I like `-allow-enabling-alpha-checks` better because it is longer and will

[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker

dcoughlin added a comment. It is really nice to see this checker take shape! Some drive by diagnostic comments in line. Comment at: test/Analysis/dangling-internal-buffer.cpp:175 std::string s; - { -c = s.c_str(); - } - consume(c); // no-warning + c = s.c_str(); //

[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:694 + } else if (Optional BE = P.getAs()) { +CFGElement BlockFront = BE->getBlock()->front(); +if (BlockFront.getKind() == CFGElement::Statement) { MTC wrote: > dcoug

[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

dcoughlin accepted this revision. dcoughlin added a comment. LGTM, but as noted inline you should update the new llvm_unreachable() to have a more descriptive error message. Do you have commit access or do you need someone to commit this for you? Comment at: lib/StaticAnalyze

[PATCH] D40463: [analyzer] Fix false negative on post-increment of uninitialized variable.

dcoughlin added a comment. Thanks for tackling this! There is one major comment inline (you are generating an extra node that you shouldn't, which is causing the analyzer to explore code it shouldn't) and a suggestion for simpler diagnostic text. Comment at: lib/StaticAnalyze

[PATCH] D40463: [analyzer] Fix false negative on post-increment of uninitialized variable.

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046 if (V2_untested.isUnknownOrUndef()) { Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested)); + lebedev.ri wrote: > dcoughlin wrote: > > Instead of gene

[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

This revision was automatically updated to reflect the committed changes. Closed by commit rL319333: [analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D37187?vs=124549&id=124778#toc Reposi

[PATCH] D40463: [analyzer] Fix false negative on post-increment of uninitialized variable.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks, this looks good to me! Do you have commit access or do you need someone to commit it for you? Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecke

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

dcoughlin added a subscriber: alexfh. dcoughlin added inline comments. Comment at: include/clang/Basic/Attr.td:602 def AnalyzerNoReturn : InheritableAttr { - let Spellings = [GNU<"analyzer_noreturn">]; + let Spellings = [Clang<"analyzer_noreturn">]; let Documentation = [Und

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

dcoughlin added inline comments. Comment at: include/clang/Basic/Attr.td:602 def AnalyzerNoReturn : InheritableAttr { - let Spellings = [GNU<"analyzer_noreturn">]; + let Spellings = [Clang<"analyzer_noreturn">]; let Documentation = [Undocumented]; aaron.bal

[PATCH] D40793: [analyzer] Improve SymbolicRegion::dump() for heap pointers.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Ship it! Repository: rC Clang https://reviews.llvm.org/D40793 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D40715: [analyser] different.LabelInsideSwitch checker implementation

dcoughlin added a comment. Thanks for looking into this! This checker is in the 'core' package, which means (when moved out of alpha) it will be enabled by default. - Do you think that this checker should be enabled by default for all users of the analyzer? - If users do actually want to use l

[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. Artem, Anna, and I discussed this a bit in person. We think that even though the benefits look great, it can't be generally applied. Maybe we could apply it in cases where othe

[PATCH] D36067: [analyzer] Create infrastructure for organizing and declaring analyzer configs.

dcoughlin added a comment. I just want to second that we really don't want to treat the analyzer-config options as user visible. These are useful as staging flags for analyzer development/testing and occasionally as customization points for IDEs/build systems. But they do not expose a coherent

[PATCH] D30406: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output

This revision was automatically updated to reflect the committed changes. Closed by commit rL309968: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D30406?vs=102838&id=109601#toc Reposi

[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

dcoughlin added a comment. It still seems like we are inferring invariants that are not sound. Do we need to restrict the symbolic transformation so that it only applies when A - B, too, is known to not overflow? Is that sufficient? Is it often the case that the analyzer knows these restriction

[PATCH] D35796: [analyzer] Misused polymorphic object checker

dcoughlin added a comment. This looks like a useful checker! Have you run this on large codebases yet? Does it find bugs? What kind of false positives do you see? Do you have a sense of what additional work would it take to bring this out of alpha and have it turned on by default? Other than s

[PATCH] D35670: [StaticAnalyzer] Handle LoopExit CFGElement in the analyzer

dcoughlin added inline comments. Comment at: include/clang/Analysis/ProgramPoint.h:658 +class LoopExit : public ProgramPoint { +public: Can you add a comment explaining what meaning of this program point is. Comment at: lib/StaticAnalyzer/Cor

[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3960-3971 + for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) { +const ParmVarDecl *Param = FD->getParamDecl(idx); +SymbolRef Sym = state->getSVal(state->getRegi

[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

dcoughlin added a comment. Nice work! This looks good, with some nits and testing comments inline. Have you run this on real code? What kind of false positives do you see? Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1853 + +void deriveParamLocation(Check

[PATCH] D35670: [StaticAnalyzer] Handle LoopExit CFGElement in the analyzer

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me! (Please expand the comment, though.) Comment at: include/clang/Analysis/ProgramPoint.h:658 +class LoopExit : public ProgramPoint { +public:

[PATCH] D35670: [StaticAnalyzer] Handle LoopExit CFGElement in the analyzer

dcoughlin added a comment. Also, please mention in the commit message that tests will be added in a following commit. https://reviews.llvm.org/D35670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

dcoughlin added a comment. Thanks! This looks good to me, with the note about factoring out the duplicated logic addressed, Would you factor out that logic into a static function and update phabricator summary to be a commit message. Then I will commit! Comment at: lib/Static

[PATCH] D36737: [analyzer] Store design discussions in docs/analyzer for future use.

dcoughlin added a comment. Thanks for doing this! https://reviews.llvm.org/D36737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.

dcoughlin added a comment. > By the way, plist-based tests in retain-release.m are disabled since r163536 > (~2012), and need to be updated. It's trivial to re-enable them but annoying > to maintain - would we prefer to re-enable or delete them or replace with > -analyzer-output=text tests? Th

[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

This revision was automatically updated to reflect the committed changes. Closed by commit rL311063: [analyzer] Add support for reference counting of parameters on the callee side (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D36441?vs=111420&id=111463#toc Reposito

[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

dcoughlin added a comment. Thanks Malhar! Committed in r311063. Repository: rL LLVM https://reviews.llvm.org/D36441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36750: [analyzer] RetainCount: When diagnosing overrelease, mention if it's coming from a nested block.

dcoughlin added a comment. I forgot to say this looks like a nice usability improvement! https://reviews.llvm.org/D36750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1104 +// expression classes separately. +if (!isa(Ex)) + for (auto Child : Ex->children()) { What is special about ObjCBoxedExpr here? Naively I would hav

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

dcoughlin added a comment. Other than my question above, this looks good to me. https://reviews.llvm.org/D35216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36851: [analyzer] Fix modeling of ctors

dcoughlin added a comment. Thanks for the patch! @NoQ and I were discussing this approach this morning. One alternative we discussed was performing this logic in RegionStore instead and skipping the default binding there if we saw that the base region was empty. What do you think of that appro

[PATCH] D69781: [analyzer] Add test directory for scan-build

dcoughlin created this revision. dcoughlin added a reviewer: NoQ. Herald added subscribers: llvm-commits, cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, delcypher, szepet, baloghadamsoftware, xazax.hun. Herald added projects: clang, LLVM. The static analyzer's

[PATCH] D69781: [analyzer] Add test directory for scan-build

This revision was automatically updated to reflect the committed changes. Closed by commit rG0aba69eb1a01: [analyzer] Add test directory for scan-build. (authored by dcoughlin). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69781/new/ https://review

[PATCH] D71728: [analyzer] Add a syntactic security check for ObjC NSCoder API.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me, but I think we need a deployment target check on the diagnostic since the safe API is only available in iOS 11+, macOS 10.13+, tvOS 11+, and watchOS 4.0+. If the d

[PATCH] D44597: [CFG] [analyzer] Add C++17-specific variable and return value construction contexts.

dcoughlin accepted this revision. dcoughlin added a comment. LGTM. I really appreciate the extra documentation you've added, too. https://reviews.llvm.org/D44597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[PATCH] D39220: [Analyzer] Store BodyFarm in std::unique_ptr

dcoughlin added a comment. I agree with Alexander here. The LLVM naming convention is not going to change and we should err on the side of using descriptive names. How about "FunctionBodyFarm"? Repository: rL LLVM https://reviews.llvm.org/D39220 __

[PATCH] D39220: [Analyzer] Store BodyFarm in std::unique_ptr

dcoughlin added a comment. In https://reviews.llvm.org/D39220#907160, @george.karpenkov wrote: > @dcoughlin OK, I'll commit that [I assuming not doing the review would be > fine here] Yes, thanks! Repository: rL LLVM https://reviews.llvm.org/D39220 _

[PATCH] D37470: [analyzer] Handle ObjC messages conservatively in CallDescription

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks Gabor. Looks great to me! https://reviews.llvm.org/D37470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D39428: [Analyzer] As suggested, use value storage for BodyFarm

dcoughlin accepted this revision. dcoughlin added a comment. Looks good to me with the comments by Gabor addressed. In https://reviews.llvm.org/D39428#912126, @george.karpenkov wrote: > On top of that, reference is part of the type, not part of the variable name, The reference is parsed as par

[PATCH] D39438: [analyzer] Diagnose stack leaks via block captures

dcoughlin added a reviewer: george.karpenkov. dcoughlin added a comment. I think this is a great idea, and I expect it to find some nasty bugs! However, I'm worried about false positives in the following not-too-uncommon idiom: dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

[PATCH] D38986: [Analyzer] Better unreachable message in enumeration

dcoughlin added a comment. Personally, I don't think this is worth it and I find it unpleasant to add untestable code -- especially if that code is going to stick around in release builds. https://reviews.llvm.org/D38986 ___ cfe-commits mailing li

[PATCH] D39551: [analyzer] Make __builtin_debugtrap() a sink

dcoughlin added a comment. I believe that the intent of `__builtin_debugtrap()` is to provide an in-source mechanism so that the developer can trap into the debugger and then continue execution. For example, in the Swift codebase this is used in combination with a debug flag to break into the d

[PATCH] D39518: [analyzer] do not crash on libcxx03 call_once implementation

dcoughlin added a comment. In https://reviews.llvm.org/D39518#914515, @george.karpenkov wrote: > Updated the diff, addressed review concerns. > Made it more explicit in comments in tests that we do not crash on libcxx03 > implementation, but that we don't model it either. > > @dcoughlin sorry I

[PATCH] D39518: [analyzer] do not crash on libcxx03 call_once implementation

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks great. Thank you! https://reviews.llvm.org/D39518 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D39577: [analyzer] [NFC] very minor ExprEngineC refactoring

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. It is a nice cleanup. https://reviews.llvm.org/D39577 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D39438: [analyzer] Diagnose stack leaks via block captures

dcoughlin added a comment. Thanks! Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing. Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:121 +QualType Q = C.getVariable()

[PATCH] D39620: [analyzer] [NFC] Remove unused typedef

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. https://reviews.llvm.org/D39620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D39584: [analyzer] [NFC] another very minor ExprEngineC refactoring

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:26 +/// results in an ElementRegion. +static void conjureOffsetSymbolOnLocation( +SVal &Symbol, SVal Other, Expr* Expression, SValBuilder &svalBuilder, I think it would be mo

[PATCH] D39691: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm

dcoughlin added a comment. Yeah, I'm not a fan of this style of testing for path diagnostics, either. https://reviews.llvm.org/D39691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast

dcoughlin created this revision. Herald added a subscriber: szepet. The ObjCGenerics checker warns on a cast when there is no subtyping relationship between the tracked type of the value and the destination type of the cast. It does this even if the cast was explicitly written. This means the user

[PATCH] D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast

dcoughlin added a comment. @xazax.hun Would you be willing to take a look? https://reviews.llvm.org/D39711 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587 if (TrackedType && + !isa(CE) && !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) && xazax.hun wrote: > george.karpenkov wrote:

[PATCH] D39682: [analyzer] Fix a crash on logical operators with vectors.

dcoughlin accepted this revision. dcoughlin added a comment. Interesting bug! The workaround looks good to me. https://reviews.llvm.org/D39682 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D39707: [analyzer] assume bitwise arithmetic axioms

dcoughlin added a comment. I believe your motivating examples used errno_t, which is a signed type. I'm fine with assuming a two's complement value representation for signed integers, which would make Artem's suggestion work. That assumption definitely deserves a comment, though. https://revi

[PATCH] D39707: [analyzer] assume bitwise arithmetic axioms

dcoughlin added a comment. This looks good to me. It is very clean! But please add a comment in the places where you are assuming a two's complement value representation for signed integers. https://reviews.llvm.org/D39707 ___ cfe-commits mailing

[PATCH] D39862: [analyzer] do not crash when trying to convert an APSInt to an unexpected type

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me. Please commit it. @NoQ can do a post-commit review and fix it up if he would rather address the issue differently. https://reviews.llvm.org/D39862 __

[PATCH] D39584: [analyzer] [NFC] another very minor ExprEngineC refactoring

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Yes, looks great. Thanks! https://reviews.llvm.org/D39584 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D39800: [analyzer] pr34404: Fix a crash on pointers to members in nested anonymous structures.

dcoughlin accepted this revision. dcoughlin added a comment. LGTM. I suppose we could move the logic that constructs pointers to members for fields here (right now that logic is in ExprEngine::VisitUnaryOperator()'s handling of '&'). However, since the AST's DeclRefExpr for the field is marked

[PATCH] D39803: [analyzer] pr34766: Fix a crash on explicit construction of std::initializer_list.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D39803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D38801: [analyzer] In getSVal() API, disable auto-detection of void type as char type.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Thanks! In https://reviews.llvm.org/D38801#910524, @NoQ wrote: > Also the overload between `getSVal(Ex, LC)` and `getSVal(R, Ty)`, when they > do completely different things, seem

[PATCH] D39964: Change code owner for Clang Static Analyzer to Devin Coughlin.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Thanks Anna! https://reviews.llvm.org/D39964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:694 + } else if (Optional BE = P.getAs()) { +CFGElement BlockFront = BE->getBlock()->front(); +if (BlockFront.getKind() == CFGElement::Statement) { MTC wrote: > szepe

[PATCH] D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks for finding and fixing this! This looks good, but since the nondeterminism is in CFG construction, I think it makes sense to create and dump the CFG rather than running the whole

[PATCH] D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses

dcoughlin added a comment. Thanks! Do you have commit access, or do you need someone to commit this for you? https://reviews.llvm.org/D40073 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses

dcoughlin added a comment. For clang itself I think we also use a stage-2 clang to build the same version of clang and make sure that it matches the stage-2 clang. This doesn't help for the analyzer though. Repository: rL LLVM https://reviews.llvm.org/D40073 _

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin created this revision. dcoughlin added reviewers: zaks.anna, NoQ. dcoughlin added subscribers: cfe-commits, alexfh, sbenza. Herald added a subscriber: mgorny. gtest is a widely-used unit-testing API provides macros for unit test assertions: ASSERT_TRUE(p != nullptr); that expand into

[PATCH] D27740: [analyzer] Include type name in Retain Count Checker diagnostics

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2000 +if (Sym->getType().isNull()) { + os << " returns an Objective-C object with a "; +} else { I think we should use this diagnostic text wh

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

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me, other than some super tiny nits! Thanks for iterating on this -- it is awesome that the analyzer will be able to model this now!! Comment at: include

[PATCH] D27740: [analyzer] Include type name in Retain Count Checker diagnostics

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D27740 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

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

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good. While you are here, you might consider changing: the checkBind() diagnostic to match the other diagnostics: "Null assigned to a pointer which is expected to have non-null va

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin marked 2 inline comments as done. dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:30 +// +// The gtest unit testing API provides macros for assertions that that expand +// into an if statement that calls a series of constructors

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin updated the summary for this revision. dcoughlin updated this revision to Diff 81651. dcoughlin marked an inline comment as done. dcoughlin added a comment. Update to address Aleksei's comments. https://reviews.llvm.org/D27773 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL289873: [analyzer] Add a new SVal to support pointer-to-member operations. (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D25475?vs=81598&id=81655#toc Repository: rL LLVM

[PATCH] D27849: crash in MallocChecker

This revision was automatically updated to reflect the committed changes. Closed by commit rL289970: [analyzer] Fix crash in MallocChecker. (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D27849?vs=81751&id=81774#toc Repository: rL LLVM https://reviews.llvm.org/D2

[PATCH] D28023: [analyzer] Fix leak false positives before no-return functions caused by incomplete analyses.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. With the change Aleksei suggested (can you get the CFGStmtMap from the AnalysisDeclContext?), looks good to me. I especially like the test! Comment at: lib/StaticAnal

[PATCH] D28033: [analyzer] Treat pointers to static member functions as function pointers

dcoughlin created this revision. dcoughlin added a reviewer: kromanenkov. dcoughlin added subscribers: cfe-commits, NoQ, zaks.anna. Sema treats pointers to static member functions as having function pointer type, so treat treat them as function pointer values in the analyzer as well. This prevents

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin added a comment. I already committed this, but I'll address the feedback in a follow-on commit. Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:172 +const CXXConstructorCall *Call, CheckerContext &C) const { + assert(Call->getNumArgs() > 0); + --

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin added a comment. In https://reviews.llvm.org/D27773#629171, @dcoughlin wrote: > I already committed this, but I'll address the feedback in a follow-on commit. I should have written "I already committed this, *so* I'll address the feedback in a follow-on commit." https://reviews.llv

[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

dcoughlin added a comment. I cleaned up the parameter detection and added more tests in r290352. https://reviews.llvm.org/D27773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me, aside from minor quibbles about capitalization and variable naming. Comment at: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp:502 +

[PATCH] D28495: [analyzer] Support inlining of '[self classMethod]' and '[[self class] classMethod]'

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:972 + Receiver == getSelfSVal().getAsRegion()) +return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl())); + Here is a case where dispatching via the compil

[PATCH] D28033: [analyzer] Treat pointers to static member functions as function pointers

dcoughlin updated this revision to Diff 83828. dcoughlin added a comment. Updating spacing, as @kromanenkov requested. https://reviews.llvm.org/D28033 Files: lib/StaticAnalyzer/Core/SValBuilder.cpp test/Analysis/pointer-to-member.cpp Index: test/Analysis/pointer-to-member.cpp

[PATCH] D28033: [analyzer] Treat pointers to static member functions as function pointers

This revision was automatically updated to reflect the committed changes. Closed by commit rL291581: [analyzer] Treat pointers to static member functions as function pointers (authored by dcoughlin). Changed prior to commit: https://reviews.llvm.org/D28033?vs=83828&id=83830#toc Repository: r

[PATCH] D31289: [analyzer] Fix symbolication for unknown unary increment/decrement results.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. https://reviews.llvm.org/D31289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

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

dcoughlin added a comment. Dominic: I don't have a bot set up yet, but let's get this committed. Thanks for all your hard work on this! https://reviews.llvm.org/D28952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Yay! This is going to fix some really confusing false positives.LGTM with Gabor's comments addressed. One note. I am not a big fan of using clang_analyzer_explain() in tests for functio

[PATCH] D28946: [analyzer] Fix memory space for block-captured static locals.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! It would be good to get this into the clang 4.0 svn branch, too. I think Hans Wennborg is managing this. See http://clang-developers.42468.n3.nabble.com/4-0-0-Release-The-branch-i

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

dcoughlin added a comment. This is super-exciting work! Some high-level notes: - The running-time numbers you report are very high. At a ~20x slowdown, the benefits from improved solver reasoning will have to be very, very large to justify the performance cost. It is worth thinking about ways

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

dcoughlin added a comment. In https://reviews.llvm.org/D28952#655278, @ddcc wrote: > > - That said, I think there are still significant optimization > > opportunities. It looks like when performing a query you create a new > > solver for each set of constraints. My understanding (and perhaps @n

[PATCH] D28946: [analyzer] Fix memory space for block-captured static locals.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM with some minor comments. Comment at: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp:98 +const VarDecl *VD = VR->getDecl(); +// FIXME: These should have

[PATCH] D29384: [analyzer] Fix an assertion fail in CStringSyntaxChecker

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. Thanks for fixing this! Repository: rL LLVM https://reviews.llvm.org/D29384 ___ cfe-commits mailing list cfe-comm

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

dcoughlin added a comment. This looks good! You will need to add tests though. I would suggest adding them to "test/Analysis/retain-release-inline.m" Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1898 +bool +isAnnotatedToSkipDiagnostics(const LocationContext *

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

dcoughlin added a comment. Thanks for the tests. Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect? I think it would be good to add some tests that reflect the reference counting implementation

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

dcoughlin added a comment. Thanks for the tests. Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect? I think it would be good to add some tests that reflect the reference counting implementation

[PATCH] D15031: CFG: Add CFGElement for automatic variables that leave the scope

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Sorry for the long delay! This looks good to me. Do you have commit access, or do you need someone to commit it for you? > Regarding " I think it would also be good to (eventually) add C

[PATCH] D16403: Add scope information to CFG

dcoughlin added a comment. > @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? These two patches are emitting markers in the CFG for different things. Here is how I would characterize the difference bet

[PATCH] D35186: [analyzer] Add annotation for functions taking user-facing strings

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Thanks for the patch! This looks very good to me. The one thing I would suggest is renaming 'isAnnotatedAsLocalized()' and 'isAnnotatedTakingLocalized()' to make it more clear what each

<    1   2   3   >