[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

Szelethus added a comment. Also, `AnalyzerOptions.def` was recently clan-formatted, feel free to run it again after the changes you make in it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135

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

Szelethus added a comment. Okay. I was wrong, and you were right. `MallocChecker` seriously suffers from overcrowding. Especially with the addition of `InnerPointerChecker`, it should be split up, but doing that is very much avoidable for the purposes of this effort while still achieving every

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

Szelethus added a comment. Yup, I agree with everything that was said. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55125/new/ https://reviews.llvm.org/D55125 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus added a reviewer: alexfh. Szelethus added a comment. *advanced summoning* CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54401/new/ https://reviews.llvm.org/D54401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

Szelethus added a subscriber: donat.nagy. Szelethus added a comment. I see your point, but here's why I think it isn't a bug: I like to see macros as `constexpr` variables, and if I used those instead, I personally wouldn't like to get a warning just because they have the same value. In C,

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

This revision was automatically updated to reflect the committed changes. Szelethus marked an inline comment as done. Closed by commit rC348038: [analyzer] Emit an error for invalid -analyzer-config inputs (authored by Szelethus, committed by ). Changed prior to commit:

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

Szelethus marked 6 inline comments as done. Szelethus added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:367 - parseAnalyzerConfigs(Opts, Diags); + if (Opts.ShouldEmitErrorsOnInvalidConfigValue) +parseAnalyzerConfigs(Opts, );

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC348031: [analyzer] Evaluate all non-checker config options before analysis (authored by Szelethus, committed by ). Changed prior to commit: https://reviews.llvm.org/D53692?vs=172856=176183#toc

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

Szelethus added a comment. Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there? Comment at: lib/CrossTU/CrossTranslationUnit.cpp:59 +} +} + `// end of namespace llvm` Comment at:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348025: [analyzer][PlistMacroExpansion] Part 5.: Support for # and ## (authored by Szelethus, committed by ). Herald added subscribers: llvm-commits, gamesh411. Changed prior to commit:

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. The idea is great! I think this should rather be an -analyzer-config flag, since the actual analysis changes with a new output (refer to `AnalyzerOption`'s doxygen comments).

[PATCH] D55131: [CTU] Add more lit tests and better error handling

Szelethus added a comment. Also, maybe it'd be worth making a CTU directory under `test/Analysis` for CTU related test files. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55131/new/ https://reviews.llvm.org/D55131

[PATCH] D55131: [CTU] Add more lit tests and better error handling

Szelethus added inline comments. Comment at: test/Analysis/ctu-main.c:3-5 +// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c +// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt +// RUN: %clang_cc1

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

Szelethus added a comment. @JonasToth this is the `Lexer` based expression equality check I talked about in D54757#1311516 . The point of this patch is that the definition is a macro sure could be build dependent, but the macro name is not, so it

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

Szelethus added a comment. In D53692#1313956 , @NoQ wrote: > In D53692#1293781 , @NoQ wrote: > > > In D53692#1293778 , @Szelethus > > wrote: > > > > > Did you know that

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

Szelethus marked 2 inline comments as done. Szelethus added a comment. Thanks! Comment at: lib/Frontend/CompilerInvocation.cpp:363 + A->claim(); Opts.Config[key] = val; } NoQ wrote: > Maybe we can eventually turn this into an array and address

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

Szelethus accepted this revision. Szelethus added a comment. In D54557#1313382 , @NoQ wrote: > In D54557#1301896 , @Szelethus wrote: > > > Wasn't the point of this patch to turn off part of this checkers > >

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC347888: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__ (authored by Szelethus, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52986/new/

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

Szelethus added a comment. In D55051#1312666 , @baloghadamsoftware wrote: > In D55051#1312660 , @Szelethus wrote: > > > Have you seen a crash resulting from this? Is supplying a test case > > feasable? I know

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

Szelethus added a comment. Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55051/new/ https://reviews.llvm.org/D55051

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

Szelethus marked 2 inline comments as done. Szelethus added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:2348 + // Enable compatilibily mode to avoid analyzer-config related errors. + CmdArgs.push_back("-analyzer-config-compatibility-mode=true"); +

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

Szelethus requested review of this revision. Szelethus added a comment. This revision is now accepted and ready to land. Changes are no longer planned for the reasons explained in my earlier comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51866/new/

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

Szelethus marked 2 inline comments as done. Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:884 + // even if we lex a tok::comma and ParanthesesDepth == 1. + const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__"); +

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

Szelethus marked 2 inline comments as done. Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:884 + // even if we lex a tok::comma and ParanthesesDepth == 1. + const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__"); +

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

Szelethus updated this revision to Diff 175718. Szelethus added a comment. Herald added a subscriber: gamesh411. - Fixed a crash where no arguments (**not** empty arguments) were supplied to `__VA_ARGS__`. - Rebased to master. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52986/new/

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus added a comment. In D54757#1311468 , @donat.nagy wrote: > **Macros:** > > The current implementation of the check only looks at the preprocessed code, > therefore it detects the situations where the duplication is created by > macros. I added

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

Szelethus added a comment. Ping^2 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54436/new/ https://reviews.llvm.org/D54436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus added a comment. Ping^2 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54401/new/ https://reviews.llvm.org/D54401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

Szelethus added a comment. Thanks! I'll commit around friday (with the requested fixes) to leave enough time for everyone to object. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53280/new/ https://reviews.llvm.org/D53280

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

Szelethus added a comment. > @george.karpenkov Matching macros is a very non-trivial job, how would you > feel if we shipped this patch as-is, and maybe leave a TODO about adding > macro `assert`s down the line? The only solution I saw in clang-tidy was to match binary expressions as a

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

Szelethus added a comment. Polite ping. :) I'd be most comfortable landing D53692 together with this patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53280/new/ https://reviews.llvm.org/D53280

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

Szelethus added a comment. Polite ping :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54437/new/ https://reviews.llvm.org/D54437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus added a comment. I think the reason why the printed message was either along the lines of "this is 0" and "this is non-0", is that we don't necessarily know what constraint solver we're using, and adding more non-general code `BugReporter` is most probably a bad approach. How about

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

Szelethus added a comment. Hmmm, I was wrong a little bit: I realized that if I tinker with with `initializeManager` and `getEnabledCheckers` just a bit more, register checkers straight away, don't need to collect them first, and this would make sure that dependencies are registered before the

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

Szelethus marked 3 inline comments as done. Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087 if (FD->getKind() == Decl::Function) { -initIdentifierInfo(C.getASTContext()); +

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC347513: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker (authored by Szelethus, committed by ). Changed prior to commit: https://reviews.llvm.org/D33672?vs=172516=175154#toc

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

Szelethus added a comment. Herald added a subscriber: baloghadamsoftware. I'll commit tomorrow, when I have time to keep an eye out for buildbots. Thanks! https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342 + DefaultBool IsOptimistic; + MemFunctionInfoTy MemFunctionInfo; public: Szelethus wrote: > Szelethus wrote: > > MTC wrote: > > > I can't say that abstracting

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

Szelethus added a comment. And thank you for helping me along the way! :D Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342 + DefaultBool IsOptimistic; + MemFunctionInfoTy MemFunctionInfo; public: MTC wrote: > I can't say that abstracting

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

Szelethus added a comment. Let's also have the link to your most recent mail about this issue here: http://lists.llvm.org/pipermail/cfe-dev/2018-November/060038.html I re-read the mailing list conversation from 2016, @szepet's `MisusedMoveChecker` patch, so I have a better graps on whats

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

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

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

Szelethus added a comment. In https://reviews.llvm.org/D18860#1297742, @NoQ wrote: > Nope, unit tests aren't quite useful here. I can't unit-test the behavior of > API that i'm about to //remove//... right? Hmmm, can't really argue with that, can I? :D https://reviews.llvm.org/D18860

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

Szelethus added a comment. In https://reviews.llvm.org/D54563#1301893, @NoQ wrote: > 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

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

Szelethus created this revision. Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs. Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, whisperity. Repository: rC Clang

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

Szelethus added a comment. In https://reviews.llvm.org/D54466#1305305, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D54466#1297887, @NoQ wrote: > > > > Hmmm, shouldn't we add this to `MemRegion`'s interface instead? > > > This: > > > I wouldn't insist, but this does indeed sound

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

Szelethus added inline comments. Comment at: lib/AST/DeclBase.cpp:1469 assert(Pos != Map->end() && "no lookup entry for decl"); -if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND) +// Remove the decl only if it is contained. +if

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

Szelethus added a comment. I also have some very neat ideas how the split up should go, but I'd like to mature the idea in my head for just a bit longer. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:24-27 +// It also has a boolean "Optimistic" checker

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

Szelethus created this revision. Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs. Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, whisperity. I'm in the process of splitting this checker into

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

Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. Polite ping :) Repository: rC Clang https://reviews.llvm.org/D54436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus added a comment. Ping, @xazax.hun, any objections? https://reviews.llvm.org/D52795 ___ 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

Szelethus added a comment. In the meanwhile we managed to figure out where the problem lays, so if you're interested, I'm going to document it here. The problem this patch attempts to solve is that `ConditionBRVisitor` adds event pieces to the bugpath when the state has changed from the

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

Szelethus 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; NoQ wrote: > Szelethus

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

Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. Ping https://reviews.llvm.org/D54401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local

This revision was automatically updated to reflect the committed changes. Closed by commit rL347157: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local (authored by Szelethus, committed by ). Herald added subscribers: llvm-commits, gamesh411, baloghadamsoftware.

[PATCH] D53069: [analyzer][www] Update avaible_checks.html

Szelethus abandoned this revision. Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. In https://reviews.llvm.org/D53069#1274554, @george.karpenkov wrote: > If we want to be serious about this page, it really has to be auto-generated > (like clang-tidy one), but

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC347153: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once (authored by Szelethus, committed by ). Changed prior to commit:

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

Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. In https://reviews.llvm.org/D51531#1296256, @NoQ wrote: > In https://reviews.llvm.org/D51531#1286110, @Szelethus wrote: > > > Oh, and the reason why I think it would add a lot of complication: since > > only

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

Szelethus updated this revision to Diff 174495. Szelethus retitled this revision from "[analyzer] Emit a warning for unknown -analyzer-config options" to "[analyzer] Emit an error for invalid -analyzer-config inputs". Szelethus set the repository for this revision to rC Clang. Szelethus added a

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

Szelethus added a comment. In https://reviews.llvm.org/D54557#1301869, @NoQ wrote: > 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

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

Szelethus added a comment. > In https://reviews.llvm.org/D54557#1300653, @NoQ wrote: > >> 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?)

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

Szelethus added a comment. In https://reviews.llvm.org/D54557#1300581, @NoQ wrote: > 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.

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

This revision was automatically updated to reflect the committed changes. Closed by commit rC347006: [analyzer] ConversionChecker: handle floating point (authored by Szelethus, committed by ). Changed prior to commit: https://reviews.llvm.org/D52730?vs=172922=174306#toc Repository: rC Clang

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

Szelethus added a comment. Herald added a subscriber: gamesh411. In https://reviews.llvm.org/D52730#1289989, @donat.nagy wrote: > Could someone with commit rights commit this patch (if it is acceptable)? I > don't have commit rights myself. I'll do the honors. Thanks! Repository: rC Clang

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

Szelethus added a comment. Herald added a subscriber: gamesh411. In https://reviews.llvm.org/D54438#1297602, @NoQ wrote: > Hmm, makes sense. Maybe `static bool BlahBlahChecker::isApplicable(const > LangOpts )` or something like that. > > Btw, maybe instead of default constructor and

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

Szelethus added a comment. Alpha checkers, at least to developers, are clearly stating "Please finish me!", but a non-alpha, enabled-by-default checker with a flag that you'd never know about unless you looked at the source code (at least up until I fix these) sounds like it'll be forgotten

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. How about `std::list::splice`? Repository: rC Clang https://reviews.llvm.org/D54563 ___ cfe-commits mailing list

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Awesome! :D Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:385-386 + } + // Provide the caller with the classification of the object + // we've obtained

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

Szelethus added a comment. 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 essentially making it an extra maintenance work would be, in my

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

Szelethus accepted this revision. Szelethus added a comment. LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there. Repository: rC Clang https://reviews.llvm.org/D54556 ___

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

Szelethus marked 5 inline comments as done. Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:982 + int ParenthesesDepth = 1; + while (ParenthesesDepth != 0) { ++It; xazax.hun wrote: > Is the misspelling already

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

Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. @george.karpenkov Matching macros is a very non-trivial job, how would you feel if we shipped this patch as-is, and maybe leave a TODO about adding macro `assert`s down the line?

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

Szelethus added a comment. Herald added a subscriber: gamesh411. Unfortunately, I found //yet another// corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this

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

Szelethus added a comment. Herald added subscribers: gamesh411, baloghadamsoftware. Unfortunately, I found //yet another// corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I

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

Szelethus added a comment. Thanks you so much! One of the reasons why I love working on this is because the great feedback I get. I don't wanna seem annoyingly grateful, but it's been a very enjoyable experience so far. > This should not cause a warnings because it should be fine to compile >

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

Szelethus added a comment. Hmmm, shouldn't we add this to `MemRegion`'s interface instead? Repository: rC Clang https://reviews.llvm.org/D54466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

Szelethus planned changes to this revision. Szelethus added a comment. Here's how I'm planning to approach this issue: - Split `MallocChecker` and `CStringChecker` up properly, introduce `MallocCheckerBase` and `CStringCheckerBase`. I'm a little unsure about how I'll do this, but I'll share my

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

Szelethus added a comment. Herald added a subscriber: baloghadamsoftware. Allow me to disagree again -- the reason why `CheckerRegistry` and the entire checker registration process is a mess is that suboptimal solutions were added instead of making the system do well on the long term. This is

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

Szelethus added a comment. Actually, no. The main problem here is that `InnerPointerChecker` doesn't only want to register `MallocBase`, it needs to modify the checker object. In any case, either `MallocChecker.cpp` needs the definition of `InnerPointerChecker`, or vice versa. Sure, I could

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

Szelethus added a comment. In https://reviews.llvm.org/D54438#1296239, @NoQ wrote: > 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

[PATCH] D53982: Output "rule" information in SARIF

Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include

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

Szelethus added a comment. In https://reviews.llvm.org/D54438#1296155, @NoQ wrote: > Can we reduce this patch to simply introducing the dependency item in > `Checkers.td` and using it in, like, one place (eg., > `MallocChecker`/`CStringChecker` inter-op) and discuss the rest of the stuff >

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

Szelethus updated this revision to Diff 173745. Szelethus edited the summary of this revision. Szelethus added a comment. Restored the discussed note. https://reviews.llvm.org/D54401 Files: include/clang/StaticAnalyzer/Core/CheckerRegistry.h

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

Szelethus 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

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

Szelethus created this revision. Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, jfb, mikhail.ramalho, a.sidorin, mgrang, szepet, whisperity, mgorny. This patch solves the problem I explained in an earlier

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

Szelethus created this revision. Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity. Now that `CheckerRegistry` lies in `Frontend`, we can finally eliminate

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

Szelethus added a comment. In https://reviews.llvm.org/D54401#1295832, @george.karpenkov wrote: > > Also, remove diags::note_suggest_disabling_all_checkers. > > Isn't that a separate revision? Given that removing existing options is often > questionable, I'd much rather see this patch

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

Szelethus added a comment. In https://reviews.llvm.org/D54429#1295838, @george.karpenkov wrote: > What do you propose we should do with other pages on the existing website? > (OpenProjects / etc.) I think we should just move everything here, and forget about the old site, as there clearly

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

Szelethus created this revision. Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity, mgorny. Herald added a reviewer: teemperor. `ClangCheckerRegistry` is a very

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

Szelethus updated this revision to Diff 173717. Szelethus set the repository for this revision to rC Clang. Szelethus added a comment. Might as well make it a method. Repository: rC Clang https://reviews.llvm.org/D54401 Files: include/clang/Basic/DiagnosticFrontendKinds.td

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL346680: [analyzer] Drastically simplify the tblgen files used for checkers (authored by Szelethus, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D54429: Creating standard shpinx documentation for Clang Static Analyzer

Szelethus added a subscriber: xazax.hun. Szelethus added a comment. I'll read through this as soon as possible, but a HUGE thank you for this. This is what the Static Analyzer desperately needed for years. This will trivialize documenting, so conversations about the inner workings of the

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

Szelethus added a comment. Thanks for taking a look! ^-^ Comment at: lib/StaticAnalyzer/Core/CheckerRegistry.cpp:51 -/// Collects the checkers for the supplied \p opt option into \p collected. -static void collectCheckers(const CheckerRegistry::CheckerInfoList , -

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Let's continue the conversation about coding standards somewhere else. Can you please mark inlines as done? https://reviews.llvm.org/D52984

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

Szelethus updated this revision to Diff 173584. Szelethus added a comment. Also, remove `diags::note_suggest_disabling_all_checkers`. https://reviews.llvm.org/D54401 Files: include/clang/Basic/DiagnosticFrontendKinds.td include/clang/StaticAnalyzer/Core/CheckerRegistry.h

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

Szelethus created this revision. Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, MTC. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity. This bugged me for a long time, so it's time to put an end to it:

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

Szelethus added inline comments. Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59 +StringRef SourceText = +Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange), + PP.getSourceManager(),

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

Szelethus added inline comments. Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59 +StringRef SourceText = +Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange), + PP.getSourceManager(),

[PATCH] D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local

Szelethus created this revision. Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs, MTC. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity. TL;DR: `CheckerOptInfo` feels very much out of place in

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

Szelethus added inline comments. Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22 +struct Entry { + SourceLocation Loc; + std::string MacroName; +}; vmiklos wrote: > Szelethus wrote: > > This is a way too general name in my opinion.

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

Szelethus added inline comments. Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22 +struct Entry { + SourceLocation Loc; + std::string MacroName; +}; This is a way too general name in my opinion. Either include comments that describe it,

  1   2   3   4   5   6   >