[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.

dcoughlin accepted this revision. dcoughlin added a comment. LGTM! Thanks Artem. https://reviews.llvm.org/D37023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

dcoughlin added a comment. This is such a nasty bug! It is great to see a fix. I have two comments inline, one of which is just a nit. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404 + // When trying to dereference a void pointer, read the first byte. +

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

dcoughlin added a comment. Artem: Sorry it too me so long to review this! For CXXStdInitializerListExpr this looks great. However, I don't think we want ObjCBoxedExprs to escape their arguments. It looks to me like these expressions copy their argument values and don't hold on to them. =

[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants

dcoughlin requested changes to this revision. dcoughlin added a subscriber: zaks.anna. dcoughlin added a comment. This revision now requires changes to proceed. Rafael: Thanks for the patch! @NoQ, @zaks.anna, and I spoke about this off-line yesterday. While this patch improves the modeling of po

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

dcoughlin accepted this revision. dcoughlin added inline comments. This revision is now accepted and ready to land. Comment at: test/Analysis/objc-boxing.m:66 + BoxableStruct bs; + bs.str = strdup("dynamic string"); // The duped string shall be owned by val. + NSValue *val = @

[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks great to me. I do wonder if long-term we should consider removing the auto-deduction when loading from the store. On the one hand it is really nice to avoid having to specify that

[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404 + // When trying to dereference a void pointer, read the first byte. + T = Ctx.CharTy; +} NoQ wrote: > dcoughlin wrote: > > Nit: It seems a bit odd

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

dcoughlin added a comment. In https://reviews.llvm.org/D35216#888506, @NoQ wrote: > Do you think this patch should be blocked in favor of complete modeling? Please, let's get this landed. We can add more precise modeling when the need arises. https://reviews.llvm.org/D35216 __

[PATCH] D38589: [Analyzer] Run tests until the end, do not stop at the first failure

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. This looks good to me. https://reviews.llvm.org/D38589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D38589: [Analyzer] Run tests until the end, do not stop at the first failure

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Oops, wrong button! LGTM! https://reviews.llvm.org/D38589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D38702: [Analyzer] Do not segfault on unexpected call_once implementation

dcoughlin added a comment. > @dcoughlin Any advice on how to handle different stdlib implementations? > Can we conjure a separate symbol instead of relying on a particular struct > layout? > For now this implementation will simply not go inside a differently > implemented call_once. I think t

[PATCH] D38702: [Analyzer] Do not segfault on unexpected call_once implementation

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/D38702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. Apologies for the delay reviewing! As I noted inline, I'm pretty worried about the performance impact of this. Is it possible to do the analysis in a single traversal of the tr

[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me! Thanks for adding this. Do you have commit access, or do you need someone to commit it for you? https://reviews.llvm.org/D37478 ___

[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null

dcoughlin added a comment. Looks like a great start! There are a bunch of minor nits inline. The one big thing is that I think your handling of 'const char *' in `typeIsConstString()` isn't quite right. 'const char *' means that the pointed-to characters can't be modified but does allow modifi

[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. Please fix the additional nits mentioned inline and commit! Also, make sure to do a pass to update the capitalization of variables throughout the file to match the LLV

[PATCH] D38810: [Analyzer] Support bodyfarming std::call_once for libstdc++

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM with the dyn_cast mentioned inline changed. Comment at: lib/Analysis/BodyFarm.cpp:359 + ValueDecl *FieldDecl = dyn_cast(FoundDecl); bool isLambdaCall = Callb

[PATCH] D38797: [analyzer] CStringChecker: pr34460: Admit that some casts are hard to model.

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

[PATCH] D23963: [analyzer] pr28449 - Move literal rvalue construction away from RegionStore.

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. OK. Seems reasonable! https://reviews.llvm.org/D23963 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers.

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/D38877 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. This comment is unrelated to this particular patch, but since I know you're doing other work in the area I think it would also be good to have test cases for the two o

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

dcoughlin added a comment. What is the workflow where this is needed? Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output? Does llvm_unreachable() guarantee that the string construction code is completely removed from rele

[PATCH] D38922: [analyzer] LoopUnrolling: check the bitwidth of the used numbers (pr34943)

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

[PATCH] D39031: [Analyzer] Correctly handle parameters passed by reference when bodyfarming std::call_once

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Some nits inline, but looks good to me! Comment at: lib/Analysis/BodyFarm.cpp:388 + // reference. + for (unsigned int i = 2; i < D->getNumParams(); i++) { + -

[PATCH] D39201: [Analyzer] Handle implicit function reference in bodyfarming std::call_once

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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Content-wise, LGTM. There is a style nit inline. Also, can you avoid reformatting the lines that haven't changed? This will help preserve the history of the file and make it clear what c

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

dcoughlin added a comment. I think a good strategy is to look at your diffs and consider whether the benefits of normalizing the style outweigh the cost of losing the history. In this case, I think keeping the history makes sense. (Imagine you are a future maintainer and want to know when and w

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

dcoughlin added a comment. That would require going into the past and requiring everyone back then to run clang-format as well. Unfortunately they didn't -- so human judgment is needed when modifying code that doesn't obey the guidelines. Repository: rL LLVM https://reviews.llvm.org/D39208

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

dcoughlin accepted this revision. dcoughlin added a comment. LGTM. https://reviews.llvm.org/D39220 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

dcoughlin added a comment. I think it would be better to add a new "test/Analysis/block-in-critical-section.m" file rather than enabling a random alpha checker in a file that tests the analyzer core. https://reviews.llvm.org/D37470 ___ cfe-commits

[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] 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] 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 = cast(CC

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

[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 http://lists.llvm.org

[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 http://lists.llvm.org/

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

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

[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 http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D33308: [analyzer]: Improve test handling with multiple constraint managers

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. Sorry of the delay here -- and thanks for your patience. I will review the other patches this weekend. https://reviews.llvm.org/D33308 ___

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

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

[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] 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: include/clang/StaticAnal

[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 http://li

[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] 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 cfe-commits@lists.llvm.org

[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] 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] 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: include/clang/StaticAnalyzer/Core/CheckerManager.h

[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] 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 &Bldr, ExplodedNode *Pred) { // TODO: Does this deserve a custom

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

[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[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 http://lists.llvm.org/c

[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] 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() (eli

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

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

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

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

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

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

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

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

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

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

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

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

[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] 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 https://lists.ll

[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] 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] 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] 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 +llvm::raw_svector_ost

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

[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 ___ cfe-com

[PATCH] D34512: Add preliminary Cross Translation Unit support library

dcoughlin added a comment. Except for some drive-by nits, this is a high-level review. I have some high-level questions about the design of the library: 1. **How do you intend to handle the case when there are multiple function definitions with the same USR?** Whose responsibility it is to deal

[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.

dcoughlin added a comment. This seems like it is on the right track, but I think the emphasis here on lvalue-to-rvalue casts is a bit misplaced. The logic for "what is the pointer being dereferenced" and "what is the lvalue that holds the pointer being dereferenced" is mixed together in a way t

[PATCH] D34512: Add preliminary Cross Translation Unit support library

dcoughlin added a comment. > The importing alters the ASTContext, so the only way to choose between the > definitions would be via a callback that is triggered before the import is > done. What do you think? I think that could work. Another possibility would be to have a two step process. The

[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements

dcoughlin added a comment. Maxim, thanks for commandeering the patch. I apologize for the long delay reviewing! I really like the approach of retrying without scopes enabled when we hit a construct we can't handle yet. This will make is possible to turn the feature on by default (and get it ru

[PATCH] D28955: [analyzer] Enable support for symbolic extension/truncation

dcoughlin added a comment. @ddcc : When I run this I get a bunch of assertion failures. Does this depend on https://reviews.llvm.org/D28953? (Which was reverted) Is it subsumed by https://reviews.llvm.org/D35450? Is this blocking on a review of another patch on our end? https://reviews.llvm.o

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

dcoughlin added a comment. I'm worried about doing this for main(). I think many people would find it surprising if analyzing main() were to behave differently than other functions. I'm particularly worried because I think it is quite common to create a small test program with only main() to un

[PATCH] D36475: [analyzer] Add "create_sink" annotation support to MagentaHandleChecker

dcoughlin added a comment. I'm a bit surprised that this is needed for unit tests. Aren't unit tests supposed to clean up their own state? Leaking a scarce resource in one unit test can cause another unit test to fail, which can be hard to track down. Or is this for deliberately testing a scena

[PATCH] D34512: Add preliminary Cross Translation Unit support library

dcoughlin added a comment. Thanks Gabor! Some additional comments in line. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:118 + /// + /// \return Returns a map with the loaded AST Units and the declarations + /// with the definitions. Is this comme

[PATCH] D34512: Add preliminary Cross Translation Unit support library

dcoughlin added a comment. I'm OK with this going into the repo a is (although it is light on tests!), as long as we have an agreement that you'll be OK with iteration on both the interface and the implementation to handle real-world projects. More specifically, for this to work well on large/c

[PATCH] D34512: Add preliminary Cross Translation Unit support library

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. > But I also think it wouldn't be good to block this until ClanD indexing > reaching a mature state. I agree 100%. > All in all, we are willing to reuse as much of ClangD indexing as we

[PATCH] D35796: [analyzer] Delete with non-virtual destructor check

dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me! Do you have commit access, or do you need someone to commit it for you? https://reviews.llvm.org/D35796 ___ cfe-com

[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] 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] 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 http://lists.llvm.org/cgi-b

[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 cfe-commits

[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: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

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

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

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

  1   2   3   >