[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

NoQ accepted this revision. NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:677 +/// need to expanded further when it is nested inside another macro. +class MacroArgMap : public std::map { +public: Szelethus wrote: > Szelethus

[PATCH] D52984: [analyzer] Checker reviewer's checklist

NoQ added a comment. Ok, so where do we put it? :) I think we should put the everyday checklist into the checker developer manual under a title like "Making Your Checker Better" and put the out-of-alpha checklist somewhere into docs/analyzer as plain text (because that's not a sort of info you

[PATCH] D52986: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__

NoQ accepted this revision. NoQ added a comment. > haha! :D https://reviews.llvm.org/D52986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52988: [analyzer][PlistMacroExpansion] Part 5.: Support for # and ##

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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. @Charusso, i believe you have some misconception about what constraints mean and how they work. Not sure what this misconception is, so i'll make a blind guess and annoy you a little bit with a narcissistic rant and you'll have to bear me, sry! The most important thing to

[PATCH] D53276: [analyzer][NFC] Fix some incorrect uses of -analyzer-config options

NoQ accepted this revision. NoQ added a comment. Yup, seems fine, please feel free to land! The `AggressiveReport` option looks unused, we should probably remove it. Sry it took so long>< https://reviews.llvm.org/D53276 ___ cfe-commits mailing list

[PATCH] D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file

NoQ accepted this revision. NoQ added a comment. In https://reviews.llvm.org/D53277#1270141, @Szelethus wrote: > In https://reviews.llvm.org/D53277#1269960, @NoQ wrote: > > > I think this is awesome o_o > > > I'm glad you like it ^-^ I mean, do i have a choice? Like, seriously, it's... beautifu

[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149 +/// If you'd like to add a new -cc1 flag, add it to +/// include/clang/Driver/CC1Options.td, add a new field to store the value of +/// that flag in this class, and initialize it i

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

NoQ added a comment. I guess i'm running out of steam for today :) > `Opts.shouldDoThat()` -> `Opts.ShouldDoThat.getValue()` Is this really unavoidable? Like, what makes them optional when they're always non-optional? Maybe just somehow prevent un-eagerly-initialized AnalyzerConfig from appear

[PATCH] D53277: [analyzer][NFC] Collect all -analyzer-config options in a .def file

NoQ added a comment. Ugh, this breaks builds with //modules//, eg. http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12074/console I'm bad with modules, but you might consult my own failure of this kind: https://reviews.llvm.org/D15448#325856 - especially the `include/clang/module.modul

[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

NoQ accepted this revision. NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149 +/// If you'd like to add a new -cc1 flag, add it to +/// include/clang/Driver/CC1Options.td, add a new field to store the value of +/// that flag in thi

[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149 +/// If you'd like to add a new -cc1 flag, add it to +/// include/clang/Driver/CC1Options.td, add a new field to store the value of +/// that flag in this class, and initialize it i

[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149 +/// If you'd like to add a new -cc1 flag, add it to +/// include/clang/Driver/CC1Options.td, add a new field to store the value of +/// that flag in this class, and initialize it i

[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

NoQ added a comment. Ok, so what this code does is, eg., for a call of `i1.operator==(i2)` that returns a symbol `$x` of type `bool`, it conserves the sacred checker-specific knowledge that `$x` is in fact equal to `$offset(i1) == $offset(i2)`, where `$offset(i)` is the offset symbol within `It

[PATCH] D53856: [analyzer] Put llvm.Conventions back in alpha

NoQ added a comment. In https://reviews.llvm.org/D53856#1285373, @rnkovacs wrote: > In https://reviews.llvm.org/D53856#1280408, @Szelethus wrote: > > > In https://reviews.llvm.org/D53856#1279887, @NoQ wrote: > > > > > This might be also covered by @rnkovacs's string buffer escape checker - > > >

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); aaron.ballman wrote: > Szelethus wro

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); aaron.ballman wrote: > NoQ wrote: >

[PATCH] D54013: [analyzer] MallocChecker: Avoid redundant transitions.

NoQ updated this revision to Diff 172629. NoQ added a comment. Re-upload with context. Whoops. https://reviews.llvm.org/D54013 Files: lib/StaticAnalyzer/Checkers/MallocChecker.cpp Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp ==

[PATCH] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2369 ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get(); + RegionStateTy OldRS = state->get(); RegionStateTy::Factory &F = state->get_context(); Szel

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

NoQ updated this revision to Diff 172645. NoQ marked 3 inline comments as done. NoQ added a comment. Fix comments, update comments. Before i forget - also add invariant violation markers to the exploded graph, which helped me a lot with debugging this bug. https://reviews.llvm.org/D54017 File

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

NoQ added inline comments. Comment at: test/Analysis/nullability.mm:3-4 // RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nul

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

NoQ added inline comments. Comment at: test/Analysis/analyzer-config.c:4-12 void bar() {} void foo() { // Call bar 33 times so max-times-inline-large is met and // min-blocks-for-inline-large is checked for (int i = 0; i < 34; ++i) { bar(); } T

[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

NoQ added a comment. In https://reviews.llvm.org/D53701#1287007, @baloghadamsoftware wrote: > ...on iterator-adapters inlining ensures that we handle the comparison of the > underlying iterator correctly. Without inlining, `evalCall()` only works on > the outermost iterator which is not always

[PATCH] D52984: [analyzer] Checker reviewer's checklist

NoQ added a comment. Thx!! Tiny nits. Comment at: www/analyzer/checker_dev_manual.html:720 +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in +Che

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks, this is really wonderful. Comment at: test/Analysis/analyzer-config.cpp:1 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 %s -o /dev/null -analyzer-checker=core,

[PATCH] D53995: [analyzer] Drastically simplify the tblgen files used for checkers

NoQ accepted this revision. NoQ added a comment. Thx!! Burn it. > tblgen files look awesome. I get that this isn't a very strong point, but > it's a pretty sight compared to a .def file. I agree that it's aesthetically satisfying, but it is (1) inconvenient to write because that's the whole ne

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

NoQ added a comment. In https://reviews.llvm.org/D53692#1293778, @Szelethus wrote: > Did you know that clang unit tests do not run with check-clang-analysis? Well, *some* unit tests definitely do run under check-clang-analysis. Repository: rC Clang https://reviews.llvm.org/D53692 __

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

NoQ added a comment. Thanks! I don't have much to add to the review, but i very much approve this check. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54222 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

NoQ updated this revision to Diff 173487. NoQ added a comment. Add an interesting test for the `MisusedMovedObject` checker that demonstrates one more potential source of false positives caused by the zombie symbol problem. In this test there are, well, //no symbols//. Therefore, there are no d

[PATCH] D54372: [analyzer] MisusedMovedObject: NFC: Remove dead code after D18860

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, baloghadamsoftware. Since https://reviews.llvm.org/D18860 addresses the problem that some re

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

NoQ added a comment. The manual region cleanup is removed from `MisusedMovedObjectChecker` in https://reviews.llvm.org/D54372. > Another thing, since we're directly testing the functionality of > `SymbolReaper`, maybe it'd be worth to also include unit tests. Sounds like a great idea, and/or i

[PATCH] D52984: [analyzer] Checker reviewer's checklist

NoQ added a comment. In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote: > Personally, I think it's detrimental to the community for subprojects to come > up with their own coding guidelines. My preference is for the static analyzer > to come more in line with the rest of the proj

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

NoQ added inline comments. Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:177 "no analyzer checkers are associated with '%0'">; -def note_suggest_disabling_all_checkers : Note< -"use -analyzer-disable-all-checks to disable all static analyzer checkers">; ---

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Hmm. All intentions in this patch are great and i love them! I think that the refactoring work in `MallocChecker` is a bit pre-mature; i don't have a clear picture of how the ideal `MallocChecker` should look like, and i'm not sure this patch moves us into the right direct

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

NoQ added a reviewer: dcoughlin. NoQ added a subscriber: dcoughlin. NoQ added a comment. I think we should escalate this to cfe-dev first; this sounds pretty important. @dcoughlin should definitely say a word here, as the code owner. I agree that pictures and screenshots are very important to ke

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Mm, i don't understand. I mean, what prevents you from cutting it off even earlier and completely omitting that part of the patch? Somebody will get to this later in order to see how exactly does the separation needs to be performed. Repository: rC Clang https://review

[PATCH] D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Mm, ok, i admit that i don't know what specific de-duplication do we want to have and what are the usability implications of it. If we want de-duplication for the same region reported in different s

[PATCH] D54372: [analyzer] MisusedMovedObject: NFC: Remove dead code after D18860

NoQ updated this revision to Diff 173779. NoQ added a comment. Bring back the early return for destructors. Cleaning up the state might be unnecessary, but calling a destructor on a moved object is still fine and should not cause a warning. Add a test because this wasn't caught by tests. http

[PATCH] D54457: [AST] Generate unique identifiers for CXXCtorInitializer objects.

NoQ created this revision. NoQ added reviewers: george.karpenkov, rsmith. Herald added a subscriber: cfe-commits. This follows https://reviews.llvm.org/D51822 and https://reviews.llvm.org/D52113 to add a cheap way of obtaining a unique and relatively stable* numeric identifier for `CXXCtorInitia

[PATCH] D54459: [analyzer] Dump reproducible identifiers for objects under construction.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, baloghadamsoftware. This continues the work that was started in https://reviews.llvm.org/D5

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok, so the point is to fix the current checker name problem, right? I guess let's land it then :) Code looks great. I'm thinking aloud in inline comments a little bit, but don't mind me. @rnkovacs

[PATCH] D51762: First part of the calendar stuff

NoQ accepted this revision. NoQ added a comment. Whoops right sry again! https://reviews.llvm.org/D51762 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51762: First part of the calendar stuff

NoQ added a comment. Ugh i cannot close it :/ https://reviews.llvm.org/D51762 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote: > - Some checkers do not register themselves in all cases[1], which should > probably be handled at a higher level, preferably in `Checkers.td`. Warnings > could be emitted when a checker is explicitly enabled bu

[PATCH] D54488: [AST] [analyzer] NFC: Reuse code in stable ID dumping methods.

NoQ created this revision. NoQ added reviewers: george.karpenkov, rsmith. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Use the new fancy method introduced in https://reviews.llvm.org/D54486 to simplify so

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

NoQ added a comment. Hmm, makes sense. Maybe `static bool BlahBlahChecker::isApplicable(const LangOpts &LO)` or something like that. Btw, maybe instead of default constructor and `->setup(Args)` method we could stuff all of the `setup(Args)`'s arguments into the one-and-only constructor for th

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

NoQ updated this revision to Diff 173932. NoQ added a comment. In https://reviews.llvm.org/D18860#1284805, @NoQ wrote: > `RetainCountChecker` is affected, as demonstrated by the newly added test. `RetainCountChecker` is affected due to temporary insanity in the checker that was induced by trus

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

NoQ added a comment. Nope, unit tests aren't quite useful here. I can't unit-test the behavior of API that i'm about to //remove//... right? https://reviews.llvm.org/D18860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D54466: [Analyzer] [WIP] Iterator Checkers - Use the base region of C++ Base Object Regions (recursively) for iterators stored in a region

NoQ added a comment. > I marked this patch as WIP because I could not create a test-case for it. > However in real projects this patch seems to reduce false positives > significantly. False positives are hard to reduce via delta-debugging because they can be accidentally reduced into true posi

[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, Szelethus. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, baloghadamsoftware, mgorny. NoQ edited the summary of this revision. NoQ added a dependency:

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, Szelethus. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, baloghadamsoftware. This patch implements the post important part of the plan proposed in h

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

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, Szelethus. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, baloghadamsoftware. The warning piece traditionally describes the bug itself, i.e., "//The b

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. So, like, with locals it should be possible to easily and intuitively suppress the false positive, even if there is one, by simply using an extra variable, and additionally there's the `[[clang::reinitializes]]` attribute added by @xazax.hun that will help people suppress t

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, Szelethus. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, baloghadamsoftware. This covers methods that //may// reset the state depending on the value

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote: > Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm > curious how this patch behaves on that project. I'll take a look. Whoops, sry, i accidentally had a look because i misread your message

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote: > I think we should either remove the non-default functionality (which wouldn't > be ideal), or emphasise somewhere (open projects?) that there is still work > to be done, but leaving it to be forgotten and essen

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. In https://reviews.llvm.org/D54557#1299736, @xazax.hun wrote: > It would be great to have a way to extend the list of (possibly non-stl) > types to check. But I do understand that the analyzer does not have a great > way to set such configuration options right now. Do you

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ updated this revision to Diff 174475. NoQ added a comment. Give the rvalue reference test function a sensible name. https://reviews.llvm.org/D54557 Files: lib/StaticAnalyzer/Checkers/MoveChecker.cpp test/Analysis/use-after-move.cpp Index: test/Analysis/use-after-move.cpp ==

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. In https://reviews.llvm.org/D54557#1300671, @Szelethus wrote: > Nice catch, would you like to make an issue on their project or shall I? I guess it can be an outright pull request :) I'll see if i have time for that soon, please feel free to take initiative>< In https://r

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

NoQ updated this revision to Diff 174477. NoQ added a comment. Write down full messages in tests. When the message was updated from `'x' is moved'` to `Object 'x' is moved`, the tests were not updated because they kept passing because the former is still a sub-string of the latter. https://rev

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

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:385-386 + } + // Provide the caller with the classification of the object + // we've obtained here accidentally, for later use. + return OK; Szelethus wrote: > Maybe move this

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ added a comment. In https://reviews.llvm.org/D54563#1299918, @Szelethus wrote: > How about `std::list::splice`? Hmm, you mean that it leaves its argument empty? Interesting, i guess we may need a separate facility for that. I wonder how many other state-reset methods are we forgetting.

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

NoQ accepted this revision. NoQ added a comment. Thanks, this looks great! Comment at: lib/Frontend/CompilerInvocation.cpp:363 + A->claim(); Opts.Config[key] = val; } Maybe we can eventually turn this into an array and address by index that's

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

NoQ added a comment. In D54557#1301896 , @Szelethus wrote: > Wasn't the point of this patch to turn off part of this checkers > functionality because it's immature just yet? From what I understand it is > desired, but the FP rate is a little too high. I

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

NoQ added a comment. Test? Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:485 + // Substracting unsigned integers is a nightmare. + if (!SingleTy->isSignedIntegerOrEnumerationType()) I guess it's no longer only about subtracting? Repository:

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

NoQ added a comment. Whoops, i was reviewing so quickly i didn't notice other comments :D I still believe that this needs a regression test, for the same reason for which we need tests at all. If the deadline for committing comes before you manage to reduce the test case, please commit, but ple

[PATCH] D50256: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager (for == and != only)

NoQ added a comment. I guess that's roughly the way to go if we want to expand `RangeConstraintManager` with more math. One important thing to test here, that doesn't seem to be tested, is what happens when the integer and the scale value are of different types. This may occur because we don't

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:502 + + auto AssertM = callExpr(callee(functionDecl(hasName("assert"; + auto GuardM = Szelethus wrote: > Szelethus wrote: > > NoQ wrote: >

[PATCH] D52984: [analyzer] Checker reviewer's checklist

NoQ added inline comments. Herald added a subscriber: gamesh411. Comment at: www/analyzer/checker_dev_manual.html:678 +Making Your Check Better + Probably `Checker` here as well. Comment at: www/analyzer/checker_dev_manual.html:681 +User faci

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

NoQ added a comment. There still seem to be a couple of regressions with `Assuming...` pieces, but other than that, i believe that an awesome piece of progress has been made here. I really like how it looks now. I agree with George that dropping "Knowing" from the message looks fancier.

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

NoQ added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. In D53692#1293781 , @NoQ wrote: > In D53692#1293778 , @Szelethus wrote: > > > Did you know that clang unit tests do not run with check-cl

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

NoQ added a comment. > I'd very strongly prefer to have this goldnugget copy-pasted even to a simple > txt file, and have it commited with this patch. I guess i'll do it a bit later because it needs to be cleaned up after the behavior changes with this patch. There are a few other such discussi

[PATCH] D54816: [RISCV] Mark unit tests as "requires: riscv-registered-target"

NoQ added a comment. Since this was reverted in rC347689 , is it ok to remove the empty `Driver` directory from the repo as well? Noticed by trying to do shell auto-complete for stuff like `D54816.diff` by just typing `D` :) Repository: rL LLVM CHANGES SIN

[PATCH] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2383-2384 + if (RS == OldRS) +return; + Szelethus wrote: > Hmmm, I guess we return here because if `RegionState` is unchanged, so should > be `ReallocPairs` and `FreeRetu

[PATCH] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.

NoQ marked an inline comment as done. NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2383-2384 + if (RS == OldRS) +return; + NoQ wrote: > Szelethus wrote: > > Hmmm, I guess we return here because if `RegionState` is un

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

NoQ reopened this revision. NoQ added a comment. This revision is now accepted and ready to land. Reverted as rC347956 due to http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/26658 - will have a look tomorrow. Repository: rC Clang CHAN

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

NoQ added inline comments. Comment at: clang/test/Analysis/osobject-retain-release.cpp:27 + + static void * operator new(unsigned long size); + NoQ wrote: > I think we should use `size_t` as much as possible, because this may > otherwise have weird consequences

[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

NoQ added a comment. Herald added a subscriber: gamesh411. In D53701#1288682 , @baloghadamsoftware wrote: > I am almost sure it is implossible. It is not only about iterator-adapters in > the classical sense but also any iterator that internally deals wi

[PATCH] D53754: [Analyzer] Skip symbolic regions based on conjured symbols in comparison of the containers of iterators

NoQ added a comment. What makes you think that conjured symbols are special? Doesn't the same problem appear in the following example?: void foo(std::vector &v1, std::vector &v2) { v2.erase(v1.cbegin()); } In this example if `foo()` is analyzed as a top level function, the respective s

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Makes perfect sense to me! Comment at: test/Analysis/iterator-range.cpp:190 + auto i0 = L.begin(); + --i0; // expected-warning{{Iterator accessed outside of its range}} +} -

[PATCH] D54149: [Analyzer] [WIP] Standard C++ library functions checker for the std::find() family

NoQ added a comment. Herald added a subscriber: gamesh411. Such checker is useful. It'd be great to add a lot of "pure" functions into it, simply for the sake of pointing out that they have no side effects, i.e. avoid unnecessary invalidation. The bound-to-argument constraint is useful. It avoi

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

NoQ added a subscriber: alexfh. NoQ added a comment. The code looks good, but i vaguely remember that we might accidentally break clang-tidy integration that uses this API to enable/disable specific checkers via `-checks=-analyzer-...`. *summons @alexfh* CHANGES SINCE LAST ACTION https://re

[PATCH] D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend

NoQ accepted this revision. NoQ added a comment. Thanks for the cleanup! One thing to test here is whether this works (or, at least, builds) when compiling clang without the analyzer. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54436/new/ https://reviews.llvm

[PATCH] D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yay! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54437/new/ https://reviews.llvm.org/D54437 ___ cfe-commits mailing

[PATCH] D54466: [Analyzer] Iterator Checkers - Use the region of the topmost base class for iterators stored in a region

NoQ added a comment. Huh, gotcha :) The nomenclature is mostly correct, just feels weird. Super-region of a region is a larger region. Sub-region is a smaller region. Base region is the largest non-memspace superregion, so it's a //larger// region. Derived class object is a large object. Base

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yay. Wow. Cool. Thanks. I heard that previously there was an explicit desire to avoid doxygen comments in checkers because they're quite useless because the checker is usually all in one file and d

[PATCH] D54834: [analyzer][MallocChecker] Improve warning messages on double-delete errors

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks, nice catch! It seems that the `ReportDoubleDelete()` thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called. Is that old code e

[PATCH] D55007: [Analyzer] Constraint Manager - Calculate Effective Range for Differences

NoQ added a comment. Is it an option to canonicalize the expression so that `B - A` was never stored in the first place? I.e., do this range intersection at the moment of writing the range, not at the moment of reading the range. This could be implemented by, say, comparing symbol IDs for `A` a

[PATCH] D54017: [analyzer] NullabilityChecker: Invariant violation should only be triggered for symbols.

NoQ added a comment. Wasn't me: rC347970 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54017/new/ https://reviews.llvm.org/D54017 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D54457: [AST] Generate unique identifiers for CXXCtorInitializer objects.

NoQ updated this revision to Diff 176465. NoQ added a comment. Rebase on top of D54486 ! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54457/new/ https://reviews.llvm.org/D54457 Files: include/clang/AST/DeclCXX.h lib/AST/DeclBase.cpp lib/AST/DeclC

[PATCH] D54457: [AST] Generate unique identifiers for CXXCtorInitializer objects.

NoQ updated this revision to Diff 176470. NoQ added a comment. Wait, i already did that in D54488 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54457/new/ https://reviews.llvm.org/D54457 Files: include/clang/AST/DeclCXX.h lib/AST/DeclCXX.cpp Ind

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

NoQ updated this revision to Diff 176504. NoQ marked 4 inline comments as done. NoQ added a comment. Address comments! In D54560#1304155 , @Charusso wrote: > In D54560#1301870 , @NoQ wrote: > > > Write down full me

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

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:385-386 + } + // Provide the caller with the classification of the object + // we've obtained here accidentally, for later use. + return OK; Szelethus wrote: > NoQ wrote: > > S

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ added a comment. In D54563#1306358 , @Szelethus wrote: > This suggests to me that `list1.splice(list1.begin(), std::move(list2))` > leaves `list2` in a valid, well-defined, but empty state. I think this should work automagically. Because `list2` is

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ updated this revision to Diff 176512. NoQ added a comment. Add a TODO. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54563/new/ https://reviews.llvm.org/D54563 Files: lib/StaticAnalyzer/Checkers/MoveChecker.cpp test/Analysis/use-after-move.cpp Index: test/Analysis/use-after-m

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ added inline comments. Comment at: test/Analysis/use-after-move.cpp:331 for (int i = 0; i < bignum(); i++) { // expected-note {{Loop condition is true. Entering loop body}} expected-note {{Loop condition is true. Entering loop body}} constCopyOrMoveCall(std::m

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ added a comment. I'm glad you brought this stuff up, found two more bugs to fix. In D54563#1317678 , @Szelethus wrote: > Can you add tests for that just in case? :) The test works, but i noticed that we aren't respecting const references. That is,

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

NoQ added a comment. In D54560#1302051 , @MTC wrote: > > The "moved-from" terminology we adopt here still feels a bit weird to me, > > but i don't have a better suggestion, so i just removed the single-quotes > > so that to at least feel proud about what

[PATCH] D54466: [Analyzer] Iterator Checkers - Use the region of the topmost base class for iterators stored in a region

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54466/new/ https://reviews.llvm.org/D54466 ___ cfe-commits mailing list cfe

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

NoQ added a comment. Hmm, i shouldn't have included `shrink_to_fit`. It definitely keeps the object in unspecified state if it is already in unspecified state. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54563/new/ https://reviews.llvm.org/D54563 __

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