[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

[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/

[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] D61958: [analyzer] RetainCount: Fix a crash when os_returns_retained_on_zero (_nonzero) is applied to functions that return weird stuff.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good do me. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61958/new/ https://reviews.llvm.org/D61958

[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61925/new/ https://reviews.llvm.org/D61925

[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.

dcoughlin accepted this revision. dcoughlin added a comment. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61545/new/ https://reviews.llvm.org/D61545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D61161: [analyzer] RetainCount: Add a suppression for functions that follow "the Matching rule".

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61161/new/ https://reviews.llvm.org/D61161

[PATCH] D60988: [analyzer] Fix crash when returning C++ objects from ObjC messages-to-nil.

dcoughlin added a comment. Looks good to me. This is a really interesting corner of Objective-C++! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60988/new/ https://reviews.llvm.org/D60988 ___ cfe-commits mailing

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:20 +// back from that variable. Test what happens if default bindings are used. +class

[PATCH] D59465: [analyzer] Add example plugin for checker option handling

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I'm not sure we really want to add any more examples of plugins. Analyzer plugins aren't supported and likely never will be. We probably shouldn't give people false hope that

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

dcoughlin added a comment. This looks good to me. It is great to see this tested! Did you consider separating the checker options from the non-checker options in the config dumper output? That would probably be easier to read. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57922/new/

[PATCH] D57860: [analyzer] Validate checker option names and values

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I'm pretty worried about exposing this flag to end users. - Almost none of the options you've listed are user facing. Many represent options intended for use by static

[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/

This revision was automatically updated to reflect the committed changes. Closed by commit rL356308: [analyzer] Teach scan-build to find clang when installed in /usr/local/bin/ (authored by dcoughlin, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/

dcoughlin updated this revision to Diff 190931. dcoughlin edited the summary of this revision. dcoughlin added a comment. Herald added a subscriber: jdoerfert. Update to restrict the new behavior to when Xcode is present. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59406/new/

[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/

dcoughlin added a comment. That's a good point. I've updated the patch to look for 'xcrun' CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59406/new/ https://reviews.llvm.org/D59406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/

dcoughlin created this revision. dcoughlin added a reviewer: NoQ. Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a project: clang. Change scan-build to support the scenario where

[PATCH] D59123: [analyzer] RetainCount: Fix a crash when a function follows retain/autorelease naming convention but takes no arguments.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Sigh. Looks good! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59123/new/ https://reviews.llvm.org/D59123

[PATCH] D58529: [analyzer] MIGChecker: Enable by default.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58529/new/ https://reviews.llvm.org/D58529 ___

[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Aah, MIG_NO_REPLY. LGTM. Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:42 + std::vector> Deallocators = { +#define CALL(required_args,

[PATCH] D58392: [analyzer] MIGChecker: Fix false negatives for releases in automatic destructors.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Oh, interesting. I'm glad you thought of this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58392/new/ https://reviews.llvm.org/D58392

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. I have some a minor diagnostic wording suggestion in line. Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:109 +

[PATCH] D58366: [analyzer] MIGChecker: Make use of the server routine annotation.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/Analysis/mig.mm:2 +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\ +// RUN:-fblocks -verify %s

[PATCH] D58365: [attributes] Add a MIG server routine attribute.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me as long as Aaron is happy with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58365/new/ https://reviews.llvm.org/D58365

[PATCH] D57981: [analyzer] strlcat() syntax check: Fix an off-by-one error.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57981/new/ https://reviews.llvm.org/D57981

[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] D55875: [analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55875/new/ https://reviews.llvm.org/D55875 ___ cfe-commits

[PATCH] D55875: [analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.

dcoughlin added a comment. These seems reasonable, although it does also seem like there could be quite a few unintended consequences that we haven't discovered yet. I'm also a bit worried about the change in the analyzer's behavior on copy(). Do you have a sense of how much of an effect this

[PATCH] D55873: [analyzer] CStringChecker: Fix a crash when an argument of a weird type is encountered.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Comment at: test/Analysis/string.cpp:23 + +// A similarly weird override outside of the class. +void *memcpy(void *, const S &, size_t); I'm

[PATCH] D55804: [analyzer] C++17: Fix leak false positives when an object with destructor is returned from the top frame.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me, although I have a question inline about why you are using `makeZeroElementRegion()`. Comment at:

[PATCH] D55566: [analyzer] LiveVariables: Fix a zombie expression problem, add testing infrastructure.

dcoughlin added a comment. This looks good to me. It is great to see a dumper for this! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55566/new/ https://reviews.llvm.org/D55566 ___ cfe-commits mailing list

[PATCH] D55671: [analyzer] Don't pretend that unknown block calls have one null parameter.

dcoughlin added a comment. Looks good to me! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55671/new/ https://reviews.llvm.org/D55671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.

dcoughlin added a comment. Looks good! Some suggested minor tweaks to diagnostic text inline. Comment at: test/Analysis/use-after-move.cpp:146 +A b = std::move(a); // expected-note {{Object 'a' is moved}} +b = a; // expected-warning {{Moved-from object is

[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] 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] 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

[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] 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

[PATCH] D44059: [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern

dcoughlin added a comment. This looks good. Some minor post-commit review inline. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:615 + +def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">, + HelpText<"Checker for performance anti-pattern when using

[PATCH] D44124: [analyzer] Support destruction and lifetime-extension of inlined function return values.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Comment at: test/Analysis/lifetime-extension.cpp:301 + } + // 1. Construct the original temporary within make(), + // 2. Construct return value of make()

[PATCH] D44120: [CFG] [analyzer] Heavier CFGValueTypedCall elements.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. I'm not super happy with the name "CFGValueTypedCall" since it doesn't make it obvious that is reflects "a function call that returns a C++ object by value." Is

[PATCH] D43804: [analyzer] Enable cfg-temporary-dtors by default?

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Yay! This looks good to me. https://reviews.llvm.org/D43804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dcoughlin accepted this revision. dcoughlin added a comment. Thanks Gabor, this looks good to me. Please commit! https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43798: [analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. I have a suggestion for a slight modification of the diagnostic text inline. Comment at: test/Analysis/implicit-ctor-undef-value.cpp:12 +// missing. Specify

[PATCH] D43714: [analyzer] Don't do anything when trivial-copying an empty class object.

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

[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.

dcoughlin accepted this revision. dcoughlin added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:70 +/// by binding a smaller object within it to a reference. +bool

[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2234 + // should go away, but the assertion should remain, without the + // "DidCacheOutOnCleanup" part, of course. + bool DidCacheOutOnCleanup = false; Can you add a comment

[PATCH] D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me. https://reviews.llvm.org/D43483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.

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/D43477 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. It seems kind of sketchy to me that we're recursing over an expression to find construction contexts and then later doing it again for sub-expressions. I guess there is precedent here

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. I have a comment about simplifying createTemporaryRegionIfNeeded() (if possible) inline. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281 + +

[PATCH] D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.

dcoughlin added a comment. This looks great to me Artem! I'll be very curious to see how you extend this to handle initializer lists. Comment at: lib/Analysis/CFG.cpp:4737 +} +case ConstructionContext::SimpleVariableKind: { + const auto *DSCC =

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:939 std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame())); +// *MRPtr may still be null when the construction context for the temporary +// was not implemented.

[PATCH] D42779: [analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing.

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 you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out!

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

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] D42672: [CFG] [analyzer] Heavier CFGConstructor elements.

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

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

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.

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

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 =

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

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] D42266: [analyzer] Prevent AnalyzerStatsChecker from crash

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 ___

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

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)

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

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

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

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] D41797: [analyzer] Suppress escape of this-pointer during construction.

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

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

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] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.

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] D41250: [analyzer] Model implied cast around operator new().

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] D40560: [analyzer] Get construction into `operator new` running in simple cases.

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] D41935: [analyzer] NFC: Mark default constructors for ProgramPoints.

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

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

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:477 +bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) { + const FunctionDecl *FD = CNE->getOperatorNew(); + if (FD && !isa(FD) && !FD->isVariadic()) {

[PATCH] D41800: [analyzer] Use a custom program point for the check::NewAllocator callback.

dcoughlin accepted this revision. dcoughlin added inline comments. This revision is now accepted and ready to land. Comment at: lib/StaticAnalyzer/Core/CheckerManager.cpp:491 NodeBuilder , ExplodedNode *Pred) { // TODO: Does this deserve a custom

[PATCH] D41408: [analyzer] NFC: Fix nothrow operator new definition in a test.

dcoughlin added a comment. LGTM as well. Repository: rC Clang https://reviews.llvm.org/D41408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41406: [analyzer] Add a new checker callback, check::NewAllocator.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me with some minor nits inside (and also a request to consider factoring out common code). Comment at:

[PATCH] D41749: [analyzer] suppress nullability inference from a macro when result is used in another macro

dcoughlin accepted this revision. dcoughlin added a comment. Yes, this looks great! https://reviews.llvm.org/D41749 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41409: [analyzer] Fix intermediate diagnostics on paths that go through operator new().

dcoughlin added a comment. LGTM as well. Repository: rC Clang https://reviews.llvm.org/D41409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41478: [analyzer] Fix zero-initialization of stack VLAs under ARC.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. The tests are great!! Repository: rC Clang https://reviews.llvm.org/D41478 ___ cfe-commits mailing list

[PATCH] D40939: [analyzer] Avoid element regions of void type.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me, although I have a super nit about wording in a comment. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2181 + // One of the forbidden LValue

[PATCH] D41258: [analyzer] trackNullOrUndefValue: deduplicate path pieces for each node.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang https://reviews.llvm.org/D41258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dcoughlin added a comment. Some comments on the C++ inline. Comment at: include/clang/AST/ASTContext.h:82 class APValue; +class ASTImporter; class ASTMutationListener; Is this forward declaration needed? Comment at:

[PATCH] D40809: [WIP] [analyzer] Dump counterexample traces as C programs

dcoughlin added a comment. In https://reviews.llvm.org/D40809#954890, @NoQ wrote: > In https://reviews.llvm.org/D40809#954858, @dcoughlin wrote: > > > One possibility is to turn this into a debug checker similar to > > debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() >

[PATCH] D40841: [analyzer] Fix a crash on C++17 AST for non-trivial construction into a trivial brace initializer.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me for a quick fix that we plan to address in a more principled fashion later. However, I'd like you to add a comment at each of the places where you use the parent

[PATCH] D40809: [WIP] [analyzer] Dump counterexample traces as C programs

dcoughlin added a comment. This is seems like a very useful visualization, *especially* for loops. Can we this patch get it into a state where it can be committed and used for debugging purposes? One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That

[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

[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

[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];

[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 =

[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:

[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=124778#toc

[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

[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:

[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:

[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: >

[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] 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

[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] 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: >

[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

[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,

[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

[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

  1   2   3   >