[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

xazax.hun added inline comments. Comment at: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:24 + do \ +if (!LLVM_WITH_Z3)

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78457/new/ https://reviews.llvm.org/D78457

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:588 + // AnyX2 means that two expressions marked as `Any` are met in code, + // and there is a special column for that, for example: + // if (x >= y)

[PATCH] D77148: [analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved

xazax.hun added a comment. Overall looks good to me, I have one question inline. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1013 +addToFunctionSummaryMap( +"__buf_size_arg_constraint_mul", +Summary(ArgTypes{ConstVoidPtrTy,

[PATCH] D79430: [analyzer] StdLibraryFunctionsChecker: Add LazyRanges to support type dependent Max

xazax.hun added a comment. I am a bit unsure what the purpose of these `Max` summaries are? As far as I understand the `Max` represents the largest value for the type of the formal parameter. Do we really ever need to specify this in a summary? Isn't it always an error to pass a value that is

[PATCH] D79432: [analyzer] StdLibraryFunctionsChecker: Add summaries for libc

xazax.hun added a comment. I think testing summaries this way can be really hard to manage in the future. I see two possible ways forward to make this easier: a) Make something like https://reviews.llvm.org/D78118 that will also dump the actual summary in a textual form, not only the fact that a

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun marked an inline comment as done. xazax.hun added a comment. Thanks! I still have some nits inline, but overall the implementation looks good to me. I think, however, that we should not only focus on the implementation but on the high-level questions. Is this the way forward we want?

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

xazax.hun added a comment. Overall looks good to me some questions and nits inline. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59 enum CallEventKind { + CE_CXXDeallocator, CE_Function, Szelethus wrote: > I need to move

[PATCH] D80171: [analyzer] LoopUnrolling: fix crash when a parameter is a loop counter

xazax.hun added inline comments. Comment at: clang/test/Analysis/loop-unrolling.cpp:503 + +void arg_as_loop_counter(int i) { + for (i = 0; i < 10; ++i) { nit: we usually use `arg` for actual arguments at the call site, and use `param` for formal parameters.

[PATCH] D80171: [analyzer] LoopUnrolling: fix crash when a parameter is a loop counter

xazax.hun added reviewers: NoQ, vsavchenko. xazax.hun added a comment. Thanks for finding this bug! Adding some reviewers. I think it would be perfectly fine to unroll loops where the loop counter is a pass-by-value parameter. That should not be considered escaped. I think in case of

[PATCH] D80117: [analyzer] Introduce reasoning about symbolic remainder operator

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:633 + + // Check if LHS is 0. It's a special case when the result is guaranteed + // to be 0 no matter what RHS is (we put to the side the case when RHS is I

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:90 + + TriState GetCmpOpState(size_t CurrentOPIndex, size_t QueriedOPIndex) const { +assert(CurrentOPIndex < CmpOpCount && QueriedOPIndex <= CmpOpCount); I

[PATCH] D79420: [analyzer] Make NonNullParamChecker as dependency for StdCLibraryFunctionsChecker

xazax.hun added a comment. > If a given parameter in a FunctionDecl has a nonull attribute then the > NonNull constraint in StdCLibraryFunctionsChecker has the same effect as > NonNullParamChecker. Wait, where the diagnostic is coming from? My point is, the user should be able to turn

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

xazax.hun added a comment. Also, I see no tests :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79431/new/ https://reviews.llvm.org/D79431 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D78118: [analyzer] StdLibraryFunctionsChecker: Add option to display loaded summaries

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78118/new/ https://reviews.llvm.org/D78118

[PATCH] D79433: [analyzer] StdLibraryFunctionsChecker: Add summaries for POSIX

xazax.hun added a comment. This is cool! Some questions: 1. Have you reported those bugs to CppCheck devs? It might be useful for us as well, as they can also double-check who is right. 2. This is a really large number of summaries. At this point, I wonder if it would be worth have a separate

[PATCH] D79423: [analyzer][NFC] StdLibraryFunctionsChecker: Add empty Signatures

xazax.hun added a comment. I am a bit unsure about this change. C libraries might be consumed in C++ projects and C++ projects might be free to overload those functions. Does the effort needed to specify the signatures justify not supporting that (probably unintentional) name collisions in

[PATCH] D79425: [analyzer] StdLibraryFunctionsChecker: Add overload for adding the same summary for different names

xazax.hun added a comment. I'd prefer to have this functionality committed together its first actual use with tests. I also agree with @balazske, we should diagnose the cases when we have multiple summaries for the same function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added a comment. In D78933#2022288 , @ASDenysPetrov wrote: > Guys, > @xazax.hun, @Charusso, @steakhal, @vsavchenko, @NoQ, @baloghadamsoftware, > please, tell something. What do you think of this improvement? Sorry, I will definitely take a

[PATCH] D76287: [analysis][analyzer] Introduce the skeleton of a reaching definitions calculator

xazax.hun added a comment. In D76287#1929221 , @Szelethus wrote:d: > //Variable// could be practically anything that can be written, but due to > the nature of what we can work this, we have to make severe restrictions. > > - We don't really have a good

[PATCH] D76287: [analysis][analyzer] Introduce the skeleton of a reaching definitions calculator

xazax.hun added reviewers: gribozavr2, ymandel. xazax.hun added a comment. Herald added a subscriber: ASDenysPetrov. Added some more reviewers who might be interested. I think it is very crucial to make the intentions clear, how do you define `definition` and `variable`? Other than assignments

[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

xazax.hun updated this revision to Diff 251569. xazax.hun marked an inline comment as done. xazax.hun added a comment. - Rebase. - Address some review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72810/new/ https://reviews.llvm.org/D72810 Files:

[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

xazax.hun marked 14 inline comments as done. xazax.hun added inline comments. Comment at: clang/include/clang/AST/LifetimeAttr.h:163 +// Maps each annotated entity to a lifetime set. +using LifetimeContracts = std::map; + aaron.ballman wrote: > xazax.hun wrote:

[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:7727 +// Move function type attribute to the declarator. +case ParsedAttr::AT_LifetimeContract: aaron.ballman wrote: > xazax.hun wrote:

[PATCH] D63279: [Analyzer] Unroll for-loops where the upper boundary is a variable with know value

xazax.hun added a comment. In D63279#1939349 , @Szelethus wrote: > (note: I forgot to submit this a couple weeks ago) > > LLVM is crashing on me due to the issue mentioned in D75678 > , but on Bitcoin, Xerces, CppCheck

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added a comment. Thanks for working on this, I do believe the analyzer would greatly profit from better constraint solving capabilities. Unfortunately, we had some troubles in the past trying to improve upon the current status and we had to revert multiple patches. This is why the

[PATCH] D79415: [analyzer][MallocChecker] When modeling realloc-like functions, don't early return if the argument is symbolic

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM, thanks! I do agree that the warning message is not the best but it is not horrible either :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:588 + // AnyX2 means that two expressions marked as `Any` are met in code, + // and there is a special column for that, for example: + // if (x >= y) I have

[PATCH] D79423: [analyzer][NFC] StdLibraryFunctionsChecker: Add empty Signatures

xazax.hun added a comment. In D79423#2025001 , @martong wrote: > 1. Some function types contain non-builtin types. E.g. `FILE*`. We cannot get > this type as easily as we do with builtin types (we can get builtins simply > from the ASTContext). In case

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added a comment. In D78933#2022684 , @ASDenysPetrov wrote: > I have never run them before. How I can do this? What project to use as a > sample? Unfortunately, we do not really have a well-defined set of benchmarks. But as a first step, you

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:588 + // AnyX2 means that two expressions marked as `Any` are met in code, + // and there is a special column for that, for example: + // if (x >= y)

[PATCH] D79423: [analyzer][NFC] StdLibraryFunctionsChecker: Add empty Signatures

xazax.hun added a comment. In D79423#2022812 , @martong wrote: > I don't think that could be a concern. > Actually, redefinition of a reserved name either in the C or in the C++ > standard library is undefined behavior: I disagree. As you mentioned in

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

xazax.hun added inline comments. Herald added a subscriber: danielkiss. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:556 +const LocationContext *LC = C.getLocationContext(); +InnerPointerVal = C.getSValBuilder().conjureSymbolVal( +CallExpr,

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun accepted this revision. xazax.hun added a comment. So now all of the dependencies are landed and this should be ready for its prime time, right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598

[PATCH] D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:326 void TransferFunctions::VisitBinaryOperator(BinaryOperator *B) { + if (LV.killAtAssign && B->getOpcode() == BO_Assign) { +if (const auto *DR = dyn_cast(B->getLHS()->IgnoreParens())) {

[PATCH] D87519: [analyzer][Liveness][NFC] Enqueue the CFGBlocks post-order

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:522 - // FIXME: we should enqueue using post order. - for (const CFGBlock *B : cfg->nodes()) { + for (const CFGBlock *B : *AC.getAnalysis()) { worklist.enqueueBlock(B);

[PATCH] D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! Looks a lot cleaner this way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87518/new/ https://reviews.llvm.org/D87518

[PATCH] D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:326 void TransferFunctions::VisitBinaryOperator(BinaryOperator *B) { + if (LV.killAtAssign && B->getOpcode() == BO_Assign) { +if (const auto *DR = dyn_cast(B->getLHS()->IgnoreParens())) {

[PATCH] D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:332 if (B->isAssignmentOp()) { if (!LV.killAtAssign) return; Nit: Maybe moving this to the front of the function would simplify the code a bit. Repository: rG

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LG! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86027/new/ https://reviews.llvm.org/D86027

[PATCH] D86334: [analyzer] Remove redundant output errs

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86334/new/ https://reviews.llvm.org/D86334

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun added inline comments. Comment at: clang/test/Analysis/z3/pretty-dump.c:15 +// CHECK: "constraints": [ +// CHECK-NEXT: { "symbol": "(reg_$[[#]]) == 3", "range": "(= reg_$[[#]] #x0003)" } + } Will this test case work with non-debug builds?

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun accepted this revision. xazax.hun added a comment. This is what I had in mind, thanks! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:335 +llvm::raw_svector_ostream OS(Str); +OS << PRETTY_SYMBOL_KIND << ID; +#undef

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

xazax.hun requested changes to this revision. xazax.hun added a comment. This revision now requires changes to proceed. Please add a test case, where the unique_ptr is initialized from a pointer parameter that has no assumptions. I think that case is not handled correctly.

[PATCH] D86293: [analyzer] Add modeling of Eq operator in smart ptr

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:351 +bool SmartPtrModeling::handleEqOp(const CallEvent , + CheckerContext ) const { I'd prefer to call this AssignOp to avoid

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. In D85528#2229309 , @steakhal wrote: > It would be nice to have this fix in clang11. > Do you think it's qualified for it? Unfortunately, this is not a fix only change. This is a fix for refutation, but also a behavioral

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! I wonder whether having a virtual method for symbols to get the prefix would be cleaner (something like getKindCStr), but I do not insist. Repository: rG LLVM Github Monorepo

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun added a comment. Exactly, but you could return a StringRef to static storage. I am fine with both approach. Whichever you find cleaner. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86223/new/ https://reviews.llvm.org/D86223

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

xazax.hun added a comment. MemRegion is a popular class to instantiate in the analyzer so it looks good to me. But unless you add a comment one will probably replace the offset with an optional as the part of a refactoring. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun added a comment. In D86223#2228855 , @steakhal wrote: > I wanted to conditionally, aka. in debug configuration override the base > implementation of the SymbolData::getKindStr I see. Yeah, that does not make much sense. I was thinking about

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

xazax.hun added a comment. In D86223#2227353 , @steakhal wrote: > Eh, I'm not sure if it worth it to put these into virtual functions - as > conditionally overriding functions is not really a good idea. I am not sure I follow. What do you mean by

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

xazax.hun added a comment. Is this related to https://bugs.llvm.org/show_bug.cgi?id=44493? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86135/new/ https://reviews.llvm.org/D86135 ___ cfe-commits

[PATCH] D86029: [analyzer] Add modeling for unque_ptr::get()

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:362-363 + const auto *InnerPointVal = State->get(ThisRegion); + if (!InnerPointVal) +return; + NoQ wrote: > You'll have to

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147 -if (!move::isMovedFrom(State, ThisR)) { - // TODO: Model this case as well. At least, avoid invalidation of - // globals. - return false; +if

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun added a comment. In D78933#2054195 , @ASDenysPetrov wrote: > @xazax.hun, any thoughts? I think we should check it on some more projects. We saw vastly different analyzer behavior on other projects in the past. I think you should be able to

[PATCH] D80016: [analyzer] StdLibraryFunctionsChecker: Add support to lookup types

xazax.hun added a comment. A high level comment. Trying to match function signatures is not a unique problem! In fact, almost all of the checks the analyzer have is trying to solve the very some problem. One of the methods we have at this point is called CallDescription, see here:

[PATCH] D77148: [analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1013 +addToFunctionSummaryMap( +"__buf_size_arg_constraint_mul", +Summary(ArgTypes{ConstVoidPtrTy, SizeTy, SizeTy}, RetType{IntTy},

[PATCH] D77148: [analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1013 +addToFunctionSummaryMap( +"__buf_size_arg_constraint_mul", +Summary(ArgTypes{ConstVoidPtrTy, SizeTy, SizeTy}, RetType{IntTy},

[PATCH] D77866: [analyzer][CallAndMessage] Add checker options for each bug category

xazax.hun added a comment. In D77866#2069144 , @NoQ wrote: > Like, i mean, the tree of packages that we currently have is a wrong > abstraction. I totally agree with this. > The most ad-hoc approach that's better than the current situation would be >

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:312 + const NoteTag *getNoteTag( + std::function Cb, + bool IsPrunable = false) { The callback is taken is an rvalue reference in

[PATCH] D85319: [analyzer][RFC] Get info from the LLVM IR for precision

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Frontend/FrontendActions.cpp:30 + // markers which are used by some LLVM analysis (e.g. AliasAnalysis). + CGO.OptimizationLevel = 2; // -O2 + martong wrote: > TODO overwrite ALL optimization

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM, thanks! I think all the problems that left should be solved in a separate patch or even out of the scope for the GSoC. Comment at:

[PATCH] D85752: [Analyzer] Store the pointed/referenced type for dynamic casts

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:73 +Ty = STTPTy->getReplacementType(); + if (Ty->isPointerType()) +Ty = Ty->getPointeeType(); Is this doing what you intended? What about a reference to a

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. In D85528#2205799 , @NoQ wrote: > I expect at least one LIT test //without// `-analyzer-config > crosscheck-with-z3=true` Agreed. I think the code snippet I proposed would be a great test to include with this revision.

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. Looks reasonable to me, but I am not very familiar with the impacts of the additional casts. Do we lose some modeling power when we are using the regular constraint solver? For example, when we have constraints both on the original and the cased symbol can the

[PATCH] D82856: [analyzer][Z3-refutation] Add statistics for refutation visitor

xazax.hun added a comment. Yay! This looks good to me and I love statistics, so a huge +1. I have one question though. Isn't this counting all the reports in an equivalence class? I.e. if the analyzer finds something multiple times it will only be displayed once but here it will be counted

[PATCH] D82445: [analyzer][solver] Track symbol equivalence

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:506 + APSIntType Type(Int); + return Int == Type.getZeroValue(); +} Does the semantics if this differ from ` llvm::APInt::isNullValue`?

[PATCH] D82445: [analyzer][solver] Track symbol equivalence

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1254 +// sufficient. +return S1->get() == S2->get() && + S1->get() == S2->get(); xazax.hun wrote: > Sorry, but I am a bit confused here.

[PATCH] D82445: [analyzer][solver] Track symbol equivalence

xazax.hun accepted this revision. xazax.hun added a comment. Thanks, LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82445/new/ https://reviews.llvm.org/D82445 ___ cfe-commits mailing list

[PATCH] D83286: [analyzer][solver] Track symbol disequalities

xazax.hun added a comment. In D83286#2149912 , @vsavchenko wrote: > @xazax.hun You were interested in performance ⏫ > > These results here compare this patch together with D82445 > against **master**. This is really

[PATCH] D83877: [Analyzer] Changed in SmartPtrModeling to handle Swap

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:965 + template + void swap(unique_ptr , unique_ptr ) noexcept { +x.swap(y);

[PATCH] D83836: [Analyzer] Implementing checkRegionChanges for SmartPtrModeling

xazax.hun added inline comments. Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:962 + operator bool() const; + unique_ptr =(unique_ptr &); +}; vrnithinkumar wrote: > added this to support use case like `Q = std::move(P)` This operation

[PATCH] D83877: [Analyzer] Changed in SmartPtrModeling to handle Swap

xazax.hun added inline comments. Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:964-965 + + template + void swap(unique_ptr , unique_ptr ) noexcept { +x.swap(y); NoQ wrote: > You seem to be relying on the fact that global `std::swap`

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun accepted this revision. xazax.hun added a comment. @szelethus The patch looks good to me and I find it a welcome change that should make things easier to understand and maybe even a bit more efficient, I hope this won't break ObjC :) My discussion with Artem is orthogonal to this

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:139 + + const auto *MethodDecl = dyn_cast_or_null(OC->getDecl()); + You should never get null here due to `isStdSmartPointerClass` guarding above. I think the

[PATCH] D82445: [analyzer][solver] Track symbol equivalence

xazax.hun added a comment. I only checked the test cases and the comments so far and it looks very useful and promising. I really hope that the performance will be ok :) I'll look at the actual code changes later. Comment at:

[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:373 + ++Upper; + --Lower; + Sorry if my question is dumb, but I do not really have a mental model at this point about where do we actually handle types and

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. I always wondered if we actually need to track the liveness of exprs at all. We could just kill all subexpr at the end of the full expression without precomputing anything. But I might miss something. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82561: [analyzer][CrossTU] Lower CTUImportThreshold default value

xazax.hun added a comment. The analyzer inlines small functions within a TU regardless of the thresholds. I think it would be sensible to do the same across TUs in the case we don't do this already. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2115656 , @NoQ wrote: > > We could just kill all subexpr at the end of the full expression > > I suspect that it would be pretty bad if you, say, kill the condition of the > `if`-statement before picking the branch.

[PATCH] D82561: [analyzer][CrossTU] Lower CTUImportThreshold default value

xazax.hun added a comment. In D82561#2116194 , @gamesh411 wrote: > In D82561#2116091 , @balazske wrote: > > > That means perform a get CTU definition if the TU to be imported (where the > > function comes from) is

[PATCH] D81564: [analyzer] SATest: Add posibility to download source from git and zip

xazax.hun added inline comments. Comment at: clang/utils/analyzer/ProjectMap.py:14 +class DownloadType(str, Enum): +GIT = "git" I was wondering what is the point of inheriting from `str`. I am not well-versed in Python so it is more of a curiosity.

[PATCH] D81315: [Draft] [Prototype] warning for default constructed unique pointer dereferences

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:46 +}; +} // end of anonymous namespace + You can merge the two anonymous namespaces, this and the one below. Comment at:

[PATCH] D81315: [Draft] [Prototype] warning for default constructed unique pointer dereferences

xazax.hun added a comment. Hi! Please add the [analyzer] tag in front of your patches as some folks have automated scripts based on that tag to add themselves as subscriber/reviewer. A small debugging/productivity tip, if you add a `printState` method to your checker like in

[PATCH] D78933: [analyzer] RangeConstraintManager optimizations in comparison expressions

xazax.hun accepted this revision. xazax.hun added a comment. I would not call the results of the measurement within the margin of error but the results do not look bad. Unless there is some objection from someone else I am ok with committing this but please change the title of revision before

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35 bool isNullAfterMoveMethod(const CallEvent ) const; + BugType NullDereferenceBugType{this, "Null-smartPtr-deref", + "C++ smart pointer"};

[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

xazax.hun added a comment. I think adding code snippets that are affected by this patch would make it much easier to evaluate the changes and whether this is a good idea or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84736/new/

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:317-320 for (Stmt *Child : S->children()) { -if (Child) - AddLiveStmt(val.liveStmts, LV.SSetFact, Child); +if (const auto *E = dyn_cast_or_null(Child)) +

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2164363 , @Szelethus wrote: > Actually, what I said initially is true: > > > [...] the only non-expression statements it **queried** are > > ObjCForCollectionStmts [...] Btw, sorry. I somehow misunderstood the

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

xazax.hun added a comment. In D82598#2162239 , @Szelethus wrote: > I chased my own tail for weeks before realizing that there is indeed another > instance when a live **statement** is stored, other then > `ObjCForCollectionStmt`... > > void

[PATCH] D94476: [analyzer] Implement conversion from Clang diagnostics to PathDiagnostics.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h:38 + + std::map> + PartialPDs; A pointer pair might be small enough for a DenseMap? Comment at:

[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92432/new/ https://reviews.llvm.org/D92432 ___ cfe-commits mailing list

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Yay! Deleting code is always nice. Comment at: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:148 nameSET_PTR_VAR_TO_NULL -

[PATCH] D93223: [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:129 +Plugins(plugins), Injector(injector), CTU(CI), +MacroExpansions(PP, CI.getLangOpts()) { DigestAnalyzerOptions(); This will always record

[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

xazax.hun added a comment. Overall looks good to me, I have some minor nits and questions inline. Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:22 +: PP(PP), SM(PP.getSourceManager()), LangOpts(LangOpts) { + class MacroExpansionRangeRecorder : public

[PATCH] D93110: [analyzer] Implement a first version of suppressions via attributes

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2897 + ASTContext ) { + PathDiagnosticLocation Location = BR.getLocation(); + What will this location return? In

[PATCH] D90362: scan-build supprot relative 'file' in cdb.

xazax.hun added a comment. In D90362#2369394 , @phosek wrote: > What's the plan regarding updating scan-build-py to match the upstream? It > seems like this issue has already been address in the upstream version. As far as I understand, the author is no

[PATCH] D90362: scan-build supprot relative 'file' in cdb.

xazax.hun added a comment. I know that the current situation is a mess, but there is an alternative version of scan-build-py on Github, which is also distributed on pypi. Could you check if that version is also susceptible to this problem and open a pull request for the author in case it is?

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

xazax.hun added a comment. In D93224#2497632 , @steakhal wrote: > Do you think it's acceptable? > @xazax.hun @NoQ It is fine by me. I believe, currently is Ericsson the biggest contributor and user of the CTU functionality. So in case they are onboard

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

xazax.hun added a comment. In D93224#2495404 , @steakhal wrote: > Do we really promise ABI compatibility for AST dumps? If we support it to > some extent, then we would have a problem - which would mean that we can not > target clang-12 anymore with

<    5   6   7   8   9   10   11   12   13   14   >