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

whisperity added a comment. Apart from those in the in-line comments, I have a question: how safe is this library to `Release` builds? I know this is only a submodule dependency for the "real deal" in https://reviews.llvm.org/D30691, but I have seen some asserts that "imported function should

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

whisperity added inline comments. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42 +/// Note that this class also implements caching. +class CrossTranslationUnit { +public: xazax.hun wrote: > whisperity wrote: > > Does the name of this class make

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

whisperity requested changes to this revision. whisperity added a comment. This revision now requires changes to proceed. Considering the results published in the opening description: /home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:55:29:

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

whisperity resigned from this revision. whisperity added inline comments. Comment at: docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst:10 + +**Case 1: Fill value is a character '0'** + Shouldn't this `'0'` be enclosed within backticks?

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

whisperity added inline comments. Comment at: clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp:57-58 +void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult ) { + // Case 1: fill_char of memset() is a character '0'. Probably an integer zero + // was intended.

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

whisperity added a comment. In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote: > In https://reviews.llvm.org/D34512#800499, @klimek wrote: > > > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote: > > > > > It looks like Richard approved libTooling as a dependency for clang on

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

whisperity added inline comments. Comment at: lib/AST/ASTContext.cpp:1497 + auto FnUnitCacheEntry = FunctionAstUnitMap.find(MangledFnName); + if (FnUnitCacheEntry == FunctionAstUnitMap.end()) { +if (FunctionFileMap.empty()) { danielmarjamaki wrote: > How

[PATCH] D32350: [Analyzer] Exception Checker

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:80 + bool WasReThrow; + Types CurrentThrows; // actually alive exceptions + FunctionExceptionsMap There are some comment formatting issues along these lines.

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

whisperity added a comment. The Python code here still uses `mangled name` in their wording. Does this mean this patch is yet to be updated with the USR management in the parent patch? Comment at: tools/scan-build-py/libscanbuild/analyze.py:165 +with

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

whisperity requested changes to this revision. whisperity added a comment. This revision now requires changes to proceed. In general, make sure the documentation page renders well in a browser. Mostly style and phrasing stuff inline: Comment at:

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity added a comment. Pinging this as the talk has stalled. https://reviews.llvm.org/D45094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

whisperity added a comment. While I understand extending the analyzer to cover more is a good approach, there is `-Wconversion` which seemingly covers this -- or at least the trivial case(?): #include #include void foo(unsigned x) { printf("%u\n", x); } int main() {

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity added a comment. Two minor comments. With regards to the function naming, the problem with incremental counts is that they get out of sync, like they did with `fXpY` and such, and if someone wants to keep the count incremental down the file, adding any new test will result in an

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

whisperity resigned from this revision. whisperity added a comment. Works for me but I haven't any sayings in these.  https://reviews.llvm.org/D43120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

whisperity requested changes to this revision. whisperity added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:21 +void ThrowKeywordMissingCheck::registerMatchers(MatchFinder *Finder) { + // This is a C++

[PATCH] D49228: [analyzer][UninitializedObjectChecker] Void pointer objects are casted back to their dynmic type in note message

whisperity added inline comments. Comment at: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp:290 struct IntDynTypedVoidPointerTest1 { - void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}} + void *vptr; // expected-note{{uninitialized pointee

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:88 + + if (!II->getName().equals("sort")) +return; Brrr... `equals`. StringRef has a `==` and `!=` operator which accepts string literals on the other side,

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

whisperity added a comment. The basics of the heuristics look okay as comparing pointers from non-continuous block of memory is undefined, it would be worthy to check if no compiler warning (perhaps by specifying `-W -Wall -Wextra -Weverything` and various others of these enable-all flags!) is

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity added a comment. Softly pinging this. Perhaps we could discuss this and get it in before Clang7 rides off into the sunset? https://reviews.llvm.org/D45094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

whisperity added a comment. Akin to https://reviews.llvm.org/D45094, pinging this too.  https://reviews.llvm.org/D45095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

whisperity added a subscriber: martong. whisperity added a comment. Make sure you use only the C++ projects like BitCoin, LLVM/Clang, ProtoBuf and such from the Xazax CSA Test suite because this checker is not really applicable to C projects. Generally I like the idea (personally I've ran into

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

whisperity added a comment. In https://reviews.llvm.org/D50488#1225064, @george.karpenkov wrote: > Why explicitly skip C projects? We would not be able to match `std::X` > functions anyway. Why spend time constructing matchers and running on the AST when we are sure that generating any

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a comment. https://reviews.llvm.org/D50353 has landed, so after a rebase this patch will not compile. https://reviews.llvm.org/D45050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

whisperity added a comment. Minor comments in-line, looking good on first glance. I'm not sure if we discussed, has this checker been tried on some in-the-wild examples? To see if there are some real findings or crashes? There is a good selection of projects here in this tool:

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

whisperity added a project: clang. whisperity added a comment. @Szelethus `clang-query` seems to sometimes not include matcher functions that are perfectly available in the code... I recently had some issue with using `isUserDefined()`, it was available in my code, but `clang-query` always

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

whisperity added a comment. Ping. Shouldn't let this thing go to waste. https://reviews.llvm.org/D46081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a comment. Whew, this is a big one. Generally looks good, although I would separate implementation detail functions a bit better, with perhaps more comments to move them apart a bit, it is really harsh to scroll through. Could you please re-run the findings on the projects?

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a comment. Generally grammar / phrasing things that have caught my eye. The rest of the code looks okay, bar my previous comment about better commenting / separating chunks of helper functions. Comment at:

[PATCH] D32747: [Analyzer] Iterator Checker - Part 3: Invalidation check, first for (copy) assignments

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:92 + // Whether iterator is valid + bool Valid; + Seeing that in line 106 you consider this record immutable, you might want to add a `const` on this field too.

[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters

whisperity added inline comments. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317 +def MismatchedIteratorChecker : Checker<"MismatchedIterator">, + HelpText<"Check for use of iterators of different containers where iterators of the same container are

[PATCH] D32859: [Analyzer] Iterator Checker - Part 5: Move Assignment of Containers

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1061 + // first reassign all iterator positions to the new container which + // are not past the container (thus not greater or equal to the + // current

[PATCH] D51926: [scan-build-py] Prevent crashes of CTU analysis by suppressing warnings

whisperity added a comment. Will this properly synergise across compilers with user-specified warning options, such as `-Wall -Werror`? Repository: rC Clang https://reviews.llvm.org/D51926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

whisperity updated this revision to Diff 141207. whisperity added a comment. Update to be in line with contents in dependency patch. https://reviews.llvm.org/D45095 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity updated this revision to Diff 141204. whisperity added a comment. - Use an even more explicit way with the documentation requiring that the file system should be an overlay. - Add a method to easily overlay a `FileSystem` above the real one. Repository: rC Clang

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

whisperity added a comment. @alexfh I have updated the patch. I don't have commit rights, so if you think this is good to go, could you please commit for me? Repository: rC Clang https://reviews.llvm.org/D45096 ___ cfe-commits mailing list

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity updated this revision to Diff 141206. whisperity added a comment. Simplify the patch. https://reviews.llvm.org/D45094 Files: include/clang/Basic/VirtualFileSystem.h include/clang/Tooling/Tooling.h lib/Basic/VirtualFileSystem.cpp lib/Tooling/Tooling.cpp

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

whisperity updated this revision to Diff 141309. whisperity added a comment. Added comments on what `nullptr` means at call sites. Repository: rC Clang https://reviews.llvm.org/D45096 Files: docs/HowToSetupToolingForLLVM.rst include/clang/Frontend/ASTConsumers.h

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

whisperity marked 2 inline comments as done. whisperity added a comment. It is also std-out (`llvm::outs()`) in case of `nullptr` and not std-err. Repository: rC Clang https://reviews.llvm.org/D45096 ___ cfe-commits mailing list

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity requested changes to this revision. whisperity added subscribers: gsd, dkrupp, o.gyorgy. whisperity added a comment. This revision now requires changes to proceed. Sorry, one comment has gone missing meanwhile, I'm still getting used to this interface and hit //Submit// early.

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity added a comment. @NoQ Do you reckon these tests files are too long? Perhaps the one about this inheritance, that inheritance, diamond inheritance, etc. could be split into multiple files. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317 +def

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

whisperity added inline comments. Comment at: clang-tidy/ClangTidy.cpp:91 public: - ErrorReporter(ClangTidyContext , bool ApplyFixes, -llvm::IntrusiveRefCntPtr BaseFS) - : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()), +

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:315 +IntrusiveRefCntPtr +createOverlayOnRealFilesystem(IntrusiveRefCntPtr TopFS); + ilya-biryukov wrote: > NIT: I'm not an expert in English, but shouldn't it be >

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

whisperity updated this revision to Diff 141194. whisperity added a comment. - Overload removed, now only one `CreateASTDumper` function remains. - Updated the call sites of this function to use this call. Repository: rC Clang https://reviews.llvm.org/D45096 Files:

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity added a comment. In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote: > In https://reviews.llvm.org/D45532#1068647, @dkrupp wrote: > > > This bug report also mentions assignment operator. But for that a warning > > may be not so useful. In that case the members of the

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity requested changes to this revision. whisperity added a comment. This revision now requires changes to proceed. There is something that came up in my mind: Consider a construct like this: class A { A() { memset(X, 0, 10 * sizeof(int)); } int X[10]; }; I

[PATCH] D45407: [StaticAnalyzer] Added notes to the plist output

whisperity added a comment. @NoQ The problem with emitting notes as events is that we lose the information that the node was a `node`. How does Xcode behave with these notes? Does it ignore them, or can read them from the command-line output of the analyser? Repository: rC Clang

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

whisperity added inline comments. Comment at: clang-tidy/tool/CMakeLists.txt:19 clangBasic + clangFrontend clangTidy ilya-biryukov wrote: > whisperity wrote: > > ilya-biryukov wrote: > > > Why do we need an extra dependency? > > In the current state of

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

whisperity added a comment. @george.karpenkov @NoQ `bugprone.` as a category sounds nice. It also nicely corresponds to the Clang-Tidy `bugprone-` category. It would not be nice to further fragment the "top levels" of checker categories. I can say with confidence that CodeChecker does not

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

whisperity created this revision. whisperity added reviewers: alexfh, klimek. whisperity added a project: clang. Herald added subscribers: dkrupp, rnkovacs. This patch extends upon https://reviews.llvm.org/D41947 because the interface that was landed from that patch isn't much user-friendly.

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

whisperity created this revision. whisperity added reviewers: alexfh, klimek, rsmith. whisperity added a project: clang. Herald added subscribers: dkrupp, rnkovacs. `ASTPrinter` allows setting the ouput to any O-Stream, but that printer creates source-code-like syntax (and is also marked with a

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a comment. @aaron.ballman Neither I nor @Charusso has commit rights, could you please commit this in our stead? https://reviews.llvm.org/D45050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

whisperity created this revision. whisperity added reviewers: rsmith, doug.gregor. whisperity added a project: clang. Herald added subscribers: Szelethus, dkrupp, rnkovacs. The current version only emits the below error for a module (attempted to be loaded) from the `prebuilt-module-path`:

[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events

whisperity accepted this revision. whisperity added a comment. This revision is now accepted and ready to land. Herald added a subscriber: donat.nagy. Remove the custom file paths and URLs but other than that this is pretty trivial. https://reviews.llvm.org/D52790

[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype

whisperity added a comment. With regards to https://reviews.llvm.org/D53352: you can change the diff related to a patch whilst keeping discuccion and metadata of the diff. Please add a generic description to the diff for an easy skimming on what you are accomplishing. If I get this right,

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

whisperity added a comment. Looks good. What happens if the macro is to stringify a partially string argument? #define BOOYAH(x) #x "; ... std::string foo = BOOYAH(blabla) std::string foo2 = BOOYAH("blabla) int x = 2; Not sure if these cases are even valid C(XX), but if they

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

whisperity added a comment. Herald added subscribers: donat.nagy, Szelethus, mikhail.ramalho, rnkovacs, szepet. Herald added a reviewer: george.karpenkov. //Soft ping.// https://reviews.llvm.org/D33672 ___ cfe-commits mailing list

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity accepted this revision. whisperity added a comment. This revision is now accepted and ready to land. Have been looked at this far and wide too many times now. I say we roll this out and fix and improve later on, lest this only going into Clang 72. Results look promising, let's hope

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

whisperity updated this revision to Diff 170832. whisperity retitled this revision from "[Frontend] Show diagnostics on prebuilt module configuration mismatch too" to "[Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too". whisperity added a comment. Herald added a

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

whisperity added a comment. @dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT invokes `clang --driver-mode=cpp` which is not the same as if `clang++` is called. I'm trying to construct the following command-line `clang++ -fmodules-ts -fprebuilt-module-path=%t/mods

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a reviewer: dyung. whisperity added a subscriber: dyung. whisperity added a comment. In https://reviews.llvm.org/D45050#1280831, @dyung wrote: > I have attached a bzipped preprocessed file that I can confirm will show the > failure if built with clang++ version 3.7.1,

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

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465 if (ChecksEnabled[CK_InvalidatedIteratorChecker] && isAccessOperator(Func->getOverloadedOperator())) { // Check for any kind of access of invalidated

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

whisperity added inline comments. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:18 +This for loop is an infinite loop because the short type can't represent all +values in the [0..size] interval. + Format the interval as code

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

whisperity added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:238 + /// message on that constraint being changed. + bool isChangedOrInsertConstraint(ConstraintMap , const Stmt *Cond, + StringRef

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

whisperity updated this revision to Diff 173124. whisperity added a comment. Yes, down the line I realised the function is not needed. (It emits a diagnostic because the diagnostic comes from comparing the AST file's config blocks to the current (at the time of import) compilation.) I have

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

whisperity added a comment. In https://reviews.llvm.org/D53974#1294385, @ztamas wrote: > I also tested on LLVm code. > The output is here: > https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4 > > I found 362 warnings. > > Around 95% of these warnings are similar to the next

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

whisperity updated this revision to Diff 172908. whisperity added a comment. Test was added. Repository: rC Clang https://reviews.llvm.org/D53334 Files: lib/Frontend/CompilerInstance.cpp test/Modules/Inputs/module-mismatch.cppm test/Modules/mismatch-diagnostics.cpp Index:

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

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1018 +auto It = CurrExpArgTokens.begin(); +while (It != CurrExpArgTokens.end()) { + if (It->isNot(tok::identifier)) { xazax.hun wrote: > Maybe a for loop mor

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

whisperity planned changes to this revision. whisperity added a comment. In https://reviews.llvm.org/D53334#1288057, @dblaikie wrote: > In https://reviews.llvm.org/D53334#1273877, @whisperity wrote: > > > @dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT > > invokes `clang

[PATCH] D52742: [analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag

whisperity added inline comments. Comment at: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:346 + + /home/eumakri/Documents/2codechecker_dev_env/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp + Same here, as

[PATCH] D52742: [analyzer][WIP] Add macro expansions to the plist output

whisperity added a subscriber: gamesh411. whisperity added a comment. Your code looks good, just minor comments going on inline. Trying to think of more cases to test for, in case someone generously misuses FLMs, as seen here in an example if you scroll down a bit:

[PATCH] D52742: [analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag

whisperity added inline comments. Comment at: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:6 + clang_version +clang version 8.0.0 (http://mainstream.inf.elte.hu/Szelethus/clang 85a6dda64587a5a18482f091cbcf020fbd3ec1dd)

[PATCH] D52390: [analyzer] Add StackSizeChecker to StaticAnalyzer

whisperity added a comment. One final nit apart from a few in-line comments (most of them are just arguing with @Szelethus anyways ): KB vs. KiB (remember how a 2 TB hard drive appears as 1.8 TB on your machine? Because it's TiB!) is one of my pet

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

whisperity added a comment. Hey @mgrang! Let's see some results on some projects and answer NoQ's comment, then I think we can put this in for all the world to see. https://reviews.llvm.org/D50488 ___ cfe-commits mailing list

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a subscriber: baloghadamsoftware. whisperity added inline comments. Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038 + + SmallString<128> NewAddNullTermExprStr; + NewAddNullTermExprStr = "\n"; aaron.ballman wrote: > This

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

whisperity added a comment. I have checked the results, thank you for uploading them, they look solid to me, although I'm not exactly a developer for these projects, without full understanding of what and where allocates and true path-sensitive analysis and memory modelling, they look good.

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

whisperity added a comment. In D35068#1361902 , @Szelethus wrote: > Edit: it doesn't, but CMake is mostly a C project and it does! CMake really isn't a C project if you look at what language it actually uses - the C files come from tests and system

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

whisperity added a comment. Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54757

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

whisperity added a comment. In https://reviews.llvm.org/D54757#1305306, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D54757#1305114, @whisperity wrote: > > > Bar the previous comments, I really like this. The test suite is massive > > and well-constructed. Do we know of any real-world

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

whisperity added a comment. Let's register the ID... Superseded by https://reviews.llvm.org/D54429. https://reviews.llvm.org/D53069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57860: [analyzer] Validate checker option names and values

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:329 + + // Insertation was successful -- CmdLineOption's constructor will validate + // whether values received from plugins or TableGen files are correct. Insertion

[PATCH] D58573: [analyzer] Move UninitializedObject out of alpha

whisperity added a comment. In case no bug reports //against// the checker are reported on the mailing list or Bugzilla, I wholeheartedly agree with kicking the ball here. As for the package, perhaps we could go into `optin.bugprone` or `optin.conventions`? (These are new package names...)

[PATCH] D58065: [analyzer] Document the frontend library

whisperity added inline comments. Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:85 +-fuse-ld=lld \ +../llvm + george.karpenkov wrote: > Information on compiling LLVM IMO should not be here. > Also, why Sphinx? Why X86? Why LLD and not

[PATCH] D57860: [analyzer] Validate checker option names and values

whisperity added inline comments. Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325 + std::string FullOption = + (llvm::Twine() + FullName + ":" + OptionName).str(); + baloghadamsoftware wrote: > Is this the most efficient way to concatenate

[PATCH] D57855: [analyzer] Reimplement checker options

whisperity added inline comments. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:49 + "NoDiagnoseCallsToSystemHeaders", + "", + "false"> Let's put at least a FIXME here that the documentation for

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

whisperity added a comment. Yeah, it seems upstream has moved away due to @Szelethus' implementation of a much more manageable "checker dependency" system. You most likely will have to rebase your patch first, then check what you missed which got added to other merged, existing checkers.

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

whisperity added a comment. I think this is good. Patch still marked as //Needs review// for some reason.  Can we look up this `blocking review` thing? Perhaps this could be marked ready to roll once the dependency patch is ironed out. Comment at:

[PATCH] D61260: [clang-tidy] Extend bugprone-sizeof-expression to check sizeof(pointers to structures)

whisperity added inline comments. Comment at: test/clang-tidy/bugprone-sizeof-expression.cpp:196 typedef const MyStruct TMyStruct; + typedef const MyStruct *PMyStruct; While I trust Clang and the matchers to unroll the type and still match, I'd prefer

[PATCH] D59465: [analyzer] Add example plugin for checker option handling

whisperity added a comment. @Szelethus I know the dependent patch D59464 will move `examples/analyzer-plugin` to `test/Analysis/plugins/...`, but this patch still seems to affect `examples/`. Are you sure this is the right diff? Because you are adding brand

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

whisperity added a comment. @dcoughlin How would removing the `USAGE` part of the dump and keeping only the list of options and their formatted help sound? That way, this option will not invite the user to directly call the analyzer. In D57858#1432714 ,

[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

whisperity added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; +

[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

whisperity accepted this revision. whisperity added a comment. @Szelethus Nope. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82 + diag(PtrArith->getBeginLoc(), + "pointer arithmetic is applied to the result of %0() instead of its " + "argument") + <<

[PATCH] D71199: [clang-tidy] New check readability-prefer-initialization-list

whisperity added a comment. Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check. Comment at:

[PATCH] D71199: [clang-tidy] New check readability-prefer-initialization-list

whisperity added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-prefer-initialization-list.cpp:221 +m = 1; +// NO-MESSAGES: initialization of m follows a conditional expression + } Comment diverged from what's

[PATCH] D71612: [analyzer] Add PlacementNewChecker

whisperity added a comment. Are buffers otherwise modelled by the Analyser, such as results of the `malloc` family and `alloca` intentionally not matched, or are there some tests missing regarding this? Comment at:

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

whisperity added a comment. I have developed a related check in D69560 . That one considers types, but is an //interface rule// checker, and does not consider (any) potential call sites. Moreover, it does not consider "swaps" that happen across a function call,

[PATCH] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

whisperity added a comment. While I can't pitch in with actual findings of this check, @MyDeveloperDay, you're right in many aspects, including those specific examples //not// firing. But an example that actually fires this check indicates a very specific **undefined behaviour** case. So if

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

whisperity added a comment. Herald added a subscriber: wuzish. A few interesting "true positive" findings: - F10568770: AnalysisDeclContext.cpp_9aaed563ddd9ebd73fdd228c2883b8e7.plist.html (Such cases with many `bool`s are being discussed on enhancing type

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

whisperity added reviewers: Szelethus, baloghadamsoftware. whisperity edited subscribers, added: baloghadamsoftware, NoQ; removed: Szelethus. whisperity added a comment. @Szelethus and @baloghadamsoftware are colleagues to me whom are far more knowledgeable about check development and I want

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

whisperity created this revision. whisperity added reviewers: aaron.ballman, alexfh, Eugene.Zelenko, JonasToth, NoQ, Szelethus, xazax.hun, baloghadamsoftware, Charusso. whisperity added a project: clang-tools-extra. Herald added subscribers: cfe-commits, ormris, kbarton, mgorny, nemanjai. Herald

  1   2   3   4   5   6   >