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

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

[PATCH] D77809: [Analyzer] Include typedef statements in CFG build.

xazax.hun requested changes to this revision. xazax.hun added a comment. This revision now requires changes to proceed. I am not an expert when it comes to VLAs but I do see some problems here. First of all, we do not want to include typedef statements in the CFG as they are noops in terms of th

[PATCH] D78233: [NFC] Correcting minor typo.

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/D78233/new/ https://reviews.llvm.org/D78233

[PATCH] D77809: [Analyzer] Include typedef statements in CFG build.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:2855 + VarDecl *VD = dyn_cast(DS->getSingleDecl()); balazske wrote: > Szelethus wrote: > > How about `using`? How about some other shenanigans that obscure the size > > of the VLA? Can'

[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: clang/lib/StaticAnalyz

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

[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. Repos

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

[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 &Call) const; + BugType NullDereferenceBugType{this, "Null-smartPtr-deref", + "C++ smart pointer"}

[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`? Comm

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

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

[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 &x, unique_ptr &y) 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 &operator=(unique_ptr &&p); +}; vrnithinkumar wrote: > added this to support use case like `Q = std::move(P)` This o

[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 &x, unique_ptr &y) noexcept { +x.swap(y); NoQ wrote: > You seem to be relying on the fact that global `std::sw

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

[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: clang/lib/StaticAnalyzer/Core/RangeConstraintManag

[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 https://reviews.llvm.org/D82

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

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

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

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

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

xazax.hun added a comment. > As turned out we don't even need a BugReporterVisitor for doing the > crosscheck. > We should simply use the constraints of the ErrorNode since that has all the > necessary information. This should not be true. But we should definitely have a test case that proves

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

xazax.hun added a comment. One way to test this change would be to add a statistic that is bumped each time a path is refuted (a report to be refuted we need all of the paths refuted, so using refuted reports might not be fine-grained enough). We can test on some big projects if the refuted pat

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

xazax.hun added a comment. In D78457#1991780 , @martong wrote: > If a symbol is unused and garbage collected then that is not part of the path > constraint that leads to the ErrorNode, is it? So why should we care about > constraints on an unused symbol?

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

xazax.hun added inline comments. Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:56 + load_threshold_reached, + ambiguous_compile_commands_database }; The two enum values refer to compilation database and compile command database. I'd prefer to

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

xazax.hun accepted this revision. xazax.hun added a comment. Ok, looks good to me. The minor nit regarding the naming is easy to fix before commit. The design question I had is not a blocker, my suggested alternative can be implemented later (if desired) in a backward-compatible way from the us

[PATCH] D77809: [Analyzer] Include typedef statements in CFG build.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Thanks, this looks much better now. Could you also update the description of the revision to match the current status? (E.g. type aliases are now supported.) If you do not plan to solve t

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

xazax.hun added a comment. Thanks! Having more tests is always welcome! I mentioned some nits inline, but I wonder if you actually need to add a new check. Can't you just reuse existing debug checks? We have the expr inspeciton checker that supports the following functions: clang_analyzer_war

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2751 +Optional hasContradictionUsingZ3(BugReporterContext &BRC, + const ExplodedNode *EndPathNode) { Is this function used anywhere?

[PATCH] D77809: [Analyzer] Include typedef statements in CFG build.

xazax.hun added a comment. Actually, sorry. I just realized that the alignof problem is introduced by this patch. I'd love to see the solution committed together with this patch (it could be a separate patch but preferably, they should be committed together.) Repository: rG LLVM Github Monor

[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] D77809: [Analyzer] Include typedef statements in CFG build.

xazax.hun accepted this revision. xazax.hun added a comment. Overall looks good for me, thanks for tackling this problem! I think this should be good to go once Eli's comment is fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77809/new/ https

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

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. I think this should not be blocked on the gtest update. If getting an updated gtest into the repo takes to much time and the reviewers are happy, I'm fine with doing that change as a fol

[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 added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2871 + // Overwrite the associated constraint of the Symbol. + Constraints = CF.remove(Constraints, Sym); Constraints = CF.add(Constraints, Sym, C.second);

[PATCH] D79072: [Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.

xazax.hun added a comment. Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs. Comment at: clang/lib/StaticAnalyzer/Cor

[PATCH] D79156: [analyzer] Merge implementations of SymInt, IntSym, and SymSym exprs

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Nice catch, LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79156/new/ https://reviews.llvm.org/D

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

[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: clang/lib/StaticAnalyzer/Chec

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

[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. Repos

[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 pointer?

[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 (ModelSm

[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:340-345 +// TODO #2: We didn't go into the nested expressions before, so it +// might cause us spending much more time doing the inference. +// This can be a problem

[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:385 + + RangeSet VisitAndOperator(RangeSet LHS, RangeSet RHS, QualType T) { +// TODO: generalize for the ranged RHS. I always get surprised when I read code

[PATCH] D79336: [analyzer] Generalize bitwise OR rules for ranges

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:77 + assert(!isEmpty()); + // NOTE: It's a shame that we can't implement 'getMaxValue' without scanning + // the whole tree to get to the last element.

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

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

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

[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 C++?

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

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

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

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

[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) xazax.hu

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

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

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

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

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

[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] D40388: [clang-tidy] rename_check.py misc-string-constructor bugprone-string-constructor

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LG! https://reviews.llvm.org/D40388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D39722: [ASTImporter] Support TypeTraitExpr Importing

xazax.hun added a comment. In https://reviews.llvm.org/D39722#933828, @aaron.ballman wrote: > In https://reviews.llvm.org/D39722#933699, @a.sidorin wrote: > > > Hello Takafumi, > > > > This is almost OK to me but there is an inline comment we need to resolve > > in order to avoid Windows buildbo

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

xazax.hun added a comment. @dcoughlin do you have some input on this? https://reviews.llvm.org/D37437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

xazax.hun added a subscriber: NoQ. xazax.hun added inline comments. Comment at: docs/ReleaseNotes.rst:261 +- Static Analyzer can now properly detect and diagnose unary pre-/post- + increment/decrement on an uninitialized values. + lebedev.ri wrote: > JonasToth w

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

xazax.hun accepted this revision. xazax.hun added a comment. I found some nit, otherwise LG! I think you should includ the context in the patches, that makes them reviewing much easier. See: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface Com

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

xazax.hun added a comment. In https://reviews.llvm.org/D38171#927046, @alexfh wrote: > And, btw, sorry for the long delay. I've been on travelling / on vacation for > the last few weeks. No problem. Will you have some time to think about the overall concept? Implementation and test wise it lo

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

xazax.hun added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:262 +std::vector ClangTidyContext::getEnabledClangDiagnostics() { + llvm::SmallVector Diags; + DiagnosticIDs::getAllDiagnostics(diag::Flavor::WarningOrError, Diags); I am wo

[PATCH] D40507: [clang-tidy] Move more checks from misc- to performance-

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Found one possible problem. Once fixed, LG! Comment at: clang-tidy/hicpp/HICPPTidyModule.cpp:33 +#include "../performance/MoveConstArgCheck.h" +#include "../performance

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

xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:64 +if (const UnaryOperator *U = dyn_cast(StoreE)) { + str = "The expression of the unary operator is an uninitialized value. " +"The computed value will a

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

xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:64 +if (const UnaryOperator *U = dyn_cast(StoreE)) { + str = "The expression of the unary operator is an uninitialized value. " +"The computed value will a

[PATCH] D38694: [ASTImporter] Support importing CXXUnresolvedConstructExpr and UnresolvedLookupExpr

xazax.hun added inline comments. Comment at: lib/AST/ASTImporter.cpp:5877 + DeclarationName Name = Importer.Import(E->getName()); + if (E->getName().isEmpty() && Name.isEmpty()) +return nullptr; Is this condition correct? https://reviews.llvm.org/D38694

[PATCH] D40787: Replace the usage of std::uncaught_exception with std::uncaught_exceptions

xazax.hun added inline comments. Comment at: test/clang-tidy/modernize-replace-uncaught-exception.cpp:41 + + res = uncaught_exception(); + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead ---

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

xazax.hun added a comment. I like the idea of adding those assertions but a bit worried about the other changes. Basically (if I get this right), we are masking the issues here and I am a bit afraid that they will get forgotten. I think it would be nice to at least add a FIXME that this path sh

[PATCH] D40937: [clang-tidy] Infinite loop checker

xazax.hun added a comment. This does not support memberExprs as condition variables right now. What happens if you have something like this: struct X { void f(int a) { while(a < i) { --i; } } int i; }; I think you could extend the test cases with some classes. ==

[PATCH] D40937: [clang-tidy] Infinite loop checker

xazax.hun added inline comments. Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:121 + + Stmt *FunctionBody = nullptr; + if (ContainingLambda) This could be pointer to const, right? https://reviews.llvm.org/D40937

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

xazax.hun added a comment. In https://reviews.llvm.org/D40939#948252, @NoQ wrote: > Like, it's not the situation when we couldn't figure out the type - it would > have been null in that case. Here we know exactly that the type is void. Oh, thank you for the clarification! Repository: rC Cl

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

xazax.hun updated this revision to Diff 125986. xazax.hun added a comment. Herald added a subscriber: rnkovacs. - @gerazo addressed some review comments in scan-build-py. https://reviews.llvm.org/D30691 Files: include/clang/AST/ASTContext.h include/clang/StaticAnalyzer/Core/AnalyzerOptions.

[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

xazax.hun added inline comments. Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:59 + if (!BeginLoc.isMacroID()) { +Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength), + "s"); aar

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

xazax.hun updated this revision to Diff 126543. xazax.hun added a comment. - Further improvements to python script part. https://reviews.llvm.org/D30691 Files: include/clang/AST/ASTContext.h include/clang/StaticAnalyzer/Core/AnalyzerOptions.h include/clang/StaticAnalyzer/Core/PathSensitiv

[PATCH] D127973: [analyzer] Eval construction of non POD type arrays.

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:493 + if (auto DS = dyn_cast_or_null(Item.getStmtOrNull())) { +if (auto VD = dyn_cast_or_null(DS->getSingleDecl())) + E = dyn_cast(VD->getInit()); Hmm, when we are

[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

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/D127898/new/ https://reviews.llvm.org/D127898 ___ cfe-commits mailing list cfe-comm

[PATCH] D122231: [clang][dataflow] Add support for `value_or` in a comparison.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:269 +// the implication `(opt.value_or(X) != X) => opt.hasValue()`. +State.Env.addToFlowCondition( +State.Env.makeImplication(*ComparisonExprValu

[PATCH] D122231: [clang][dataflow] Add support for `value_or` in a comparison.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:142 + // `opt.value_or(nullptr) != nullptr` and `opt.value_or(0) != 0`. Ideally, + // we'd support this pattern for any expression, but the AST does not have a

<    1   2   3   4   5   6   7   8   9   10   >