[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

lebedev.ri added inline comments. Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63 +configuration for accepted floating point values, primarily because most +floating point comparisons are not exact, and some of the exact ones are not +portable.

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 158030. lebedev.ri marked 9 inline comments as done. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Address last portion of @rsmith review notes. @rsmith, @vsk, @erichkeane - thank you for the review! Repository: rC Clang

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If cont

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If cont

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri created this revision. lebedev.ri added reviewers: rjmccall, rsmith, erichkeane. https://reviews.llvm.org/rC337815 / https://reviews.llvm.org/D49508 had to cannibalize one bit of `CastExprBitfields::BasePathSize` in order to squeeze `PartOfExplicitCast` boolean. That reduced the maxim

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri updated this revision to Diff 158311. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. - Store/prepend it into `llvm::TrailingObjects<>` - Use `unsigned short` - Assert that the storage is at least large enough to be compliant with implimits

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri added inline comments. Comment at: test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:1 +// RUN: %clang_cc1 %s -emit-llvm -o - + erichkeane wrote: > First, most of this test can be further simplified. Second, I'd like to see > a test that actually t

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri added inline comments. Comment at: include/clang/AST/Expr.h:2791 +public: + using BasePathSizeTy = unsigned short; + static_assert(std::numeric_limits::max() >= 16384, erichkeane wrote: > I'll let @rjmccall comment here, but I think I'd be willing to

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri updated this revision to Diff 158344. lebedev.ri marked 6 inline comments as done. lebedev.ri added a comment. Address some of @erichkeane review notes. Repository: rC Clang https://reviews.llvm.org/D50050 Files: include/clang/AST/Expr.h include/clang/AST/ExprCXX.h include/c

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri updated this revision to Diff 158379. lebedev.ri added a comment. Drop pointless cast of zero. Repository: rC Clang https://reviews.llvm.org/D50050 Files: include/clang/AST/Expr.h include/clang/AST/ExprCXX.h include/clang/AST/ExprObjC.h include/clang/AST/Stmt.h lib/AST/E

[PATCH] D50102: Use ExprMutationAnalyzer in performance-unnecessary-value-param

lebedev.ri added a comment. It is great when the test cases are included :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

lebedev.ri added a comment. In https://reviews.llvm.org/D50050#1183341, @rjmccall wrote: > LGTM. Thank you for the review. I'll land in ~+9 hours or so, since i did not hear from @erichkeane back yet.. Repository: rC Clang https://reviews.llvm.org/D50050

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri created this revision. lebedev.ri added reviewers: vsk, rsmith, rjmccall, Sanitizers. lebedev.ri added a project: Sanitizers. C and C++ are interesting languages. They are statically typed, but weakly. The implicit conversions are allowed. This is nice, allows to write code while balanc

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:2004 +if (auto *ICE = dyn_cast(CE)) { + if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitConversion) && + !ICE->isPartOfExplicitCast()) { erichkeane wrote: > Is this an er

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri updated this revision to Diff 159004. lebedev.ri marked 5 inline comments as done. lebedev.ri added a comment. Address most of @erichkeane review notes. Repository: rC Clang https://reviews.llvm.org/D50250 Files: docs/ReleaseNotes.rst docs/UndefinedBehaviorSanitizer.rst incl

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1008 + // We ignore conversions to/from pointer and/or bool. + if (!(SrcType->isIntegerType() && DstType->isIntegerType())) +return; erichkeane wrote: > I'd rather !SrcType->isInt ||

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added a project: clang. lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1008 + // We ignore conversions to/from pointer and/or bool. + if (!(SrcType->isIntegerType() && DstType->isIntegerType())) +return; erichkeane wro

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added a comment. @erichkeane, @rsmith thanks for taking a look! Comment at: lib/CodeGen/CGExprScalar.cpp:1036 +return; + // That's it. We can't rule out any more cases with the data we have. + rsmith wrote: > I don't like the overlap between the

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1036 +return; + // That's it. We can't rule out any more cases with the data we have. + rsmith wrote: > rsmith wrote: > > lebedev.ri wrote: > > > rsmith wrote: > > > > I don't like th

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri updated this revision to Diff 159187. lebedev.ri marked 11 inline comments as done. lebedev.ri added a comment. Address most of @rsmith review notes. Repository: rC Clang https://reviews.llvm.org/D50250 Files: docs/ReleaseNotes.rst docs/UndefinedBehaviorSanitizer.rst include

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1129-1130 + return; + } + // That's it. We can't rule out any more cases with the data we have. Actually, after messing with souper a little, if we are converting from *larger* *

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri updated this revision to Diff 159230. lebedev.ri added a comment. Do not emit sign-change check in `signed int -> signed char` case. The truncation check is sufficient: https://godbolt.org/g/r1wgQG https://rise4fun.com/Alive/ifj The middle-end [clearly] does not understand that, but si

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

lebedev.ri added inline comments. Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:26 + + static const llvm::StringMap Mapping{ +// [simd.alg] You probably want to move `Mapping` out of the function. Comment at: clang-tidy/readab

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

lebedev.ri added inline comments. Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:77 +void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr(callee(functionDecl(matchesName("^::(_mm_|_mm256_|_mm512_|vec_)"))),

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

lebedev.ri added inline comments. Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:75 + // libcxx implementation of std::experimental::simd requires at least C++11. + if (!Result.Context->getLangOpts().CPlusPlus11) +return; timshen wrote: > MaskRa

[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

lebedev.ri added inline comments. Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:46 + + static const llvm::StringMap Mapping{ +// [simd.alg] I think you can use `llvm::StringRef` here instead of `std::string` Comment at: clang-

[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

lebedev.ri created this revision. lebedev.ri added reviewers: rsmith, rtrieu, aaron.ballman. lebedev.ri added a project: clang. Let's suppose the `-Weverything` is passed. Given code like void F() {} ; If the code is compiled with `-std=c++03`, it would diagnose that extra sema: :2:1: wa

[PATCH] D43277: limits: Use `false` instead of `type(0)`.

lebedev.ri added a comment. In https://reviews.llvm.org/D43277#1009659, @brucem wrote: > Without this, anyone using `clang-tidy` with this check gets 6 system header > warnings whenever they transitively have included `limits`. Only if `-system-headers` is passed to clang-tidy, or if those lib

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. I don't know the protocol, but i think it might be a good idea to add a new entry to `CODE_OWNERS.TXT` for `clang-doc`? `clang-doc` going to be quite distinctive, and bigger/complicated than what already is in `clang-tools-extra`. https://reviews.llvm.org/D41102 _

[PATCH] D43424: [clang-doc] Implement a (simple) Markdown generator

lebedev.ri added a comment. It will be good to have the tests for generators. Comment at: clang-doc/generators/Generators.h:28 +public: + Generator(std::unique_ptr &IS, StringRef Root, StringRef Format) : IS(IS), Root(Root), Format(Format) {}; + virtual ~Generator() {};

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Nice work! It will be great to have a replacement for doxygen, which is actually modern and usable. Some review notes for some of the code below: Comment at: clang-doc/ClangDocBinary.cpp:17 + +enum BlockIds { + NAMESPACE_BLOCK_ID = bitc::FIRST_APP

[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D43162 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43303: [Format] Fix for bug 35641

lebedev.ri added a comment. The diff looks wrong, the `lib/Format/WhitespaceManager.cpp` change gone missing. You need to upload the diff to the `git master`/svn trunk/etc, *NOT* to the previous commit (previous diff). Repository: rC Clang https://reviews.llvm.org/D43303

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Great work! Some more review... Comment at: clang-doc/ClangDoc.cpp:32 + ECtx.reportResult( + Name, Mapper.emitInfo(D, getComment(D), Name, getLine(D), getFile(D))); +} I wonder if `Name` should be `std::move()`'d ? Or not, `rep

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/ClangDocBinary.h:82 + +static std::map BlockIdNameMap = { + {NAMESPACE_BLOCK_ID, "NamespaceBlock"}, lebedev.ri wrote: > Nice! > Some thoughts: > 1. I agree it makes sense to keep it close to the enum defini

[PATCH] D34365: [FrontEnd] Allow overriding the default C/C++ -std via CMake vars

lebedev.ri added a comment. clang 6.0 defaults to C++14, BTW. https://reviews.llvm.org/D34365 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/ClangDocBinary.cpp:72 + assert(Abbrevs.find(recordID) == Abbrevs.end() && + "Abbreviation already set."); + Abbrevs[recordID] = abbrevID; juliehockett wrote: > lebedev.ri wrote: > > lebedev.ri wrot

[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

lebedev.ri added a comment. After a brief disscussion with @aaron.ballman in IRC, the question still stands whether this is really the correct approach, or is there some nicer (TableGen-based?) approach. Mainly waiting for @rsmith's opinion. Repository: rC Clang https://reviews.llvm.org/D431

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Nice! Comment at: clang-doc/BitcodeWriter.cpp:33 + llvm::IndexedMap BlockIdNameMap; + BlockIdNameMap.resize(BI_LAST - BI_FIRST + 1); + So here's the thing. We know how many enumerators we have (`BI_LAST - BI_FIRST + 1`). An

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/BitcodeWriter.cpp:407 + +void ClangDocBinaryWriter::writeBitstream(const EnumInfo &I, + BitstreamWriter &Stream, juliehockett wrote: > lebedev.ri wrote: > > Hmm, comm

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/Representation.h:79 + std::string Filename; +}; + Hmm, have you tried adding a constructor here? ``` struct Location { int LineNumber; std::string Filename; Location() = default; Location(int Line

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/BitcodeWriter.cpp:149 + +/// \brief Emits a record ID in the BLOCKINFO block. +void ClangDocBitcodeWriter::emitRecordID(RecordId ID) { For me, with no prior knowledge of llvm's bitstreams, it was not obvious

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. An idea on how to further generalize/cleanup `emitBlockInfoBlock()`. While *i think* will help, i'm not sure how to further consolidate the `BlockIdNameMap`/`RecordIdNameMap` and the actual `emitBlock(*)`... Comment at: clang-doc/BitcodeWriter.cpp:

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

lebedev.ri added a comment. Somewhat related thing i have noticed: https://godbolt.org/g/ow58JV I wonder whether that also should be diagnosed? Where though, as clang-tidy check? Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commi

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Next, i suggest to look into code self-debugging, see comments. Also, i have added a few questions, it would be great to know that my understanding is correct? I'm sorry that it seems like we are going over and over and over over the same code again, this is the ver

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Could you please add a bit more tests? In particular, i'd like to see how blocks-in-blocks work. I.e. class-in-class, class-in-function, ... Is there some (internal to `BitstreamWriter`) logic that would 'assert()' if trying to output some recordid which is, accordin

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Thank you for working on this! Some more thoughts. Comment at: clang-doc/BitcodeWriter.cpp:191 + Record.clear(); + for (const char C : BlockIdNameMap[ID]) Record.push_back(C); + Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_BLOCKNAME, Record);

[PATCH] D43737: Improve -Winfinite-recursion

lebedev.ri added a comment. Please upload patches with full diff (-U99) Repository: rC Clang https://reviews.llvm.org/D43737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D43162 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43780: [Tooling] [1/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

lebedev.ri created this revision. lebedev.ri added reviewers: klimek, bkramer, alexfh, pcc. lebedev.ri added projects: clang, clang-tools-extra. Herald added subscribers: jkorous-apple, ioeric. I'm not sure whether there are any principal reasons why it returns raw owning pointer, or it is just a

[PATCH] D43779: [Tooling] [0/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

lebedev.ri created this revision. lebedev.ri added reviewers: klimek, bkramer, alexfh, pcc. lebedev.ri added a project: clang. Noticed during review of https://reviews.llvm.org/D41102. I'm not sure whether there are any principal reasons why it returns raw owning pointer, or it is just a old cod

[PATCH] D33497: clang-tidy check for __func__/__FUNCTION__ in lambdas

lebedev.ri added a comment. 1. What about `__PRETTY_FUNCTION__` ? 2. Consider following generic error handling macro: (ThrowException is some template function) #undef STR #define STR(a) XSTR(a) #define ThrowExceptionHelper(CLASS, fmt, ...) ThrowException(__FILE__ ":" STR(__LINE__) ":

[PATCH] D33497: clang-tidy check for __func__/__FUNCTION__ in lambdas

lebedev.ri added a comment. In https://reviews.llvm.org/D33497#763376, @brycel wrote: > In https://reviews.llvm.org/D33497#763307, @lebedev.ri wrote: > > > 1. What about `__PRETTY_FUNCTION__` ? > > > I think `__PRETTY_FUNCTION__` inside a lambda is useful enough not to warn > about. Then that

[PATCH] D33365: [clang-tidy] misc-assertion-count: A New Check

lebedev.ri added a comment. I will obviously try to add lambda handling, but until then, the general idea and design should be pretty much done, so i'd love to hear any feedback about the current diff :) Repository: rL LLVM https://reviews.llvm.org/D33365

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added inline comments. Comment at: test/clang-tidy/cert-avoid-pointer-cast-to-more-strict-alignment.c:17 + + tmp = (struct foo_header *)(data + offset); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not cast pointers into more strictly aligned pointer types [ce

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri updated this revision to Diff 101232. lebedev.ri marked an inline comment as done. lebedev.ri added a subscriber: cfe-commits. lebedev.ri added a comment. Address review notes. Repository: rL LLVM https://reviews.llvm.org/D33102 Files: docs/ReleaseNotes.rst lib/Sema/SemaCast.c

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri added a comment. In https://reviews.llvm.org/D33102#767291, @dblaikie wrote: > Still seems like an awful lot more testing than I'd expect for this warning. > (eg: testing all the many variations of C++ const_cast - when they all > provide basically the same coverage I think, that al

[PATCH] D33827: [clang-tidy] misc-static-assert shouldn't flag assert(!"msg")

lebedev.ri added a comment. While not directly related to this Differential, i wonder if it would make sense to also not warn on calls to (especially non-member) functions marked with `__attribute__((const))`, maybe `__attribute__((pure))` too. At least i'm not sure what side-effects such calls

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri added a comment. In https://reviews.llvm.org/D33102#773296, @dblaikie wrote: > I still feel like that's more testing than would be ideal (does the context > of the cast matter? Wether it's dereferenced, a struct member access, > assigned, initialized, etc - it doesn't look like it fr

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri added a comment. (tested with: gcc version 6.3.0 20170516 (Debian 6.3.0-18) ```) Repository: rL LLVM https://reviews.llvm.org/D33102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D33365: [clang-tidy] misc-assertion-count: A New Check

lebedev.ri added a comment. In https://reviews.llvm.org/D33365#775860, @alexfh wrote: > I guess, this check should go to `readability` or elsewhere, but definitely > not to `misc`. Hmm, `misc` may be a bad place for this, but i think `readability` is even worse fit. The best guess would be so

[PATCH] D32942: [clang-tidy] readability-function-size: add NestingThreshold param.

lebedev.ri added a comment. In https://reviews.llvm.org/D32942#776915, @malcolm.parsons wrote: > Does this check consider `else if` chains to be nesting? > > void nesting() { // 1 > if (true) { // 2 >int j; > } else if (true) { // 2 or 3? >int j; > } else if (true) {

[PATCH] D16717: [clang-tidy] Add non-constant references in function parameters check.

lebedev.ri added a comment. // Don't warn on dependent types in templates. Hmm, i wanted to try to fix my Bug 32683 about templated references being ignored, and i found out that it is actually intentional... Reading https://google.github.io/styl

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri planned changes to this revision. lebedev.ri added a comment. In https://reviews.llvm.org/D33102#773296, @dblaikie wrote: > But sure. Could you also (manually, I guess) confirm that this matches GCC's > cast-qual behavior (insofar as the warning fires in the same situations). If > th

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri updated this revision to Diff 102119. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. After further mail exchange, i will proceed to commit this as-is. No code changes, rebase before commit. Repository: rL

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

lebedev.ri added a comment. So i'm trying to analyze that stage2 warning. The testcase //seems// to be: (autogenerated all the variants) void test_nop() { unsigned char **ptr1 = 0; void **ptr2 = (void **)ptr1; } void test_bad() { unsigned char **ptr1 = 0; const void **ptr2 =

[PATCH] D32942: [clang-tidy] readability-function-size: add NestingThreshold param.

lebedev.ri added a comment. In https://reviews.llvm.org/D32942#778729, @malcolm.parsons wrote: > In https://reviews.llvm.org/D32942#777001, @lebedev.ri wrote: > > > Which makes sense, since in AST, they are nested: > > > They're not nested in the formatting, so I don't think it makes sense. As

[PATCH] D32942: [clang-tidy] readability-function-size: add NestingThreshold param.

lebedev.ri added a comment. In https://reviews.llvm.org/D32942#780053, @malcolm.parsons wrote: > In https://reviews.llvm.org/D32942#779143, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D32942#778729, @malcolm.parsons wrote: > > > > > In https://reviews.llvm.org/D32942#777001, @lebedev.ri

[PATCH] D32942: [clang-tidy] readability-function-size: add NestingThreshold param.

lebedev.ri added a comment. Hm, oh, maybe this is as simple as a partially-unintentional `switch` fallthrough Repository: rL LLVM https://reviews.llvm.org/D32942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri created this revision. lebedev.ri added a project: clang-tools-extra. A followup for https://reviews.llvm.org/D32942. Malcolm Parsons has provided a valid testcase that the initial version of the check complained about nested `if`'s. As it turns out, the culprit is the **partially** u

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri planned changes to this revision. lebedev.ri added a comment. Hmm, this is moving in the right direction, but now it invalidated (decreases the nesting level) too eagerly Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-com

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri updated this revision to Diff 102533. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Ok, i really messed up the initial https://reviews.llvm.org/D32942 :) Malcolm, thank you for bothering to report :) This should be so much better. Repository: rL LLVM h

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:38 +} else { + assert(!isa(Node)); + if (TrackedParent.back()) malcolm.parsons wrote: > I don't think this assert adds anything. Yes, however the original `if

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri updated this revision to Diff 102542. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Fix spelling. Repository: rL LLVM https://reviews.llvm.org/D34202 Files: clang-tidy/readability/FunctionSizeCheck.cpp test/clang-tidy/readability-function-size.cpp I

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri updated this revision to Diff 102548. lebedev.ri added a comment. Simplify it even further by moving the logic into it's proper place - `TraverseCompoundStmt()` Repository: rL LLVM https://reviews.llvm.org/D34202 Files: clang-tidy/readability/FunctionSizeCheck.cpp test/clang-

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri added a comment. In https://reviews.llvm.org/D34202#780394, @malcolm.parsons wrote: > What do you expect for this? > > if (true) > if (true) > if (true) { > int j; > } > that it is equivalent to if (true && true && true) { // 1 int j;

[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

lebedev.ri added a comment. In https://reviews.llvm.org/D34202#780424, @malcolm.parsons wrote: > My coding standards require the `{}`, so while I may not agree with your > definition of nesting, it doesn't matter. Well, seeing `readability-deep-nesting.cpp` in the issue, i suppose the definit

[PATCH] D41224: [ThreadSafetyAnalysis] Fix isCapabilityExpr

lebedev.ri added a comment. Hi. I don't want to hijack the thread, but is PR32954 likely unrelated to this fix, and the problem (if it is a bug) is likely elsewhere? Repository: rC Clang https://reviews.llvm.org/D41224 ___

[PATCH] D41455: [ASTMatchers] Add isNoReturn() match narrower for FunctionDeclarations

lebedev.ri created this revision. lebedev.ri added a reviewer: aaron.ballman. lebedev.ri added a project: clang. Herald added a subscriber: klimek. How to regenerate LibASTMatchersReference.html ? Repository: rC Clang https://reviews.llvm.org/D41455 Files: include/clang/ASTMatchers/ASTMatc

[PATCH] D41456: [clang-tidy] readability-else-after-return: also diagnose noreturn function calls.

lebedev.ri created this revision. lebedev.ri added reviewers: aaron.ballman, alexfh, djasper, malcolm.parsons. lebedev.ri added a project: clang-tools-extra. Herald added a subscriber: xazax.hun. The readability-else-after-return check was not warning about an else after a call to the function t

[PATCH] D41456: [clang-tidy] readability-else-after-return: also diagnose noreturn function calls.

lebedev.ri added a comment. Depends on https://reviews.llvm.org/D41455. I'm open to suggestions re diagnostic wording. The current `'noreturn call'` seems lazy, and is basically a placeholder. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41456 __

[PATCH] D41455: [ASTMatchers] Add isNoReturn() match narrower for FunctionDeclarations

lebedev.ri updated this revision to Diff 127780. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. - Added C tests - Cleaned-up spurious semicolons - Docs are still not regenerated, somehow that script results in a huge diff for me. Repository: rC Clang https://reviews

[PATCH] D41456: [clang-tidy] readability-else-after-return: also diagnose noreturn function calls.

lebedev.ri added a comment. In https://reviews.llvm.org/D41456#961826, @malcolm.parsons wrote: > This check could also handle else after goto. Yes, certainly. Though i'm not too sure on the restrictions. The obvious precondition is, the label can to be defined anywhere **except** after the `go

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri created this revision. lebedev.ri added reviewers: aaron.ballman, rsmith, smeenai, rjmccall, rnk, mclow.lists. lebedev.ri added a project: clang. The diagnostic was mostly introduced in https://reviews.llvm.org/D38101 by me, as a reaction to wasting a lot of time, see mail

[PATCH] D41507: avxintrin.h documentation fixes and updates

lebedev.ri added inline comments. Comment at: lib/Headers/avxintrin.h:1668 ///operation to use: \n -///0x00 : Equal (ordered, non-signaling) -///0x01 : Less-than (ordered, signaling) -///0x02 : Less-than-or-equal (ordered, signaling) -///0x03 : Unordered (non

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri added a comment. In https://reviews.llvm.org/D41512#963743, @dim wrote: > Actually, having thought about it a little more, if the warning is "rather > broken", or even "completely broken", depending on one's point of view, then > maybe it is better not have it under `-Wextra` either?

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri updated this revision to Diff 128113. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. In https://reviews.llvm.org/D41512#963743, @dim wrote: > We now still have to explicitly use `-Wno-tautological-constant-compare` > everywhere. :-( OTOH, thank you for b

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 128114. lebedev.ri added a comment. Rebased. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D36836 Files: LICENSE.TXT clang-tidy/CMakeLists.txt clang-tidy/plugin/CMakeLists.txt clang-tidy/sonarsource/CMakeLists.txt clang-tidy/so

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:439 def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">; def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">; +de

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri updated this revision to Diff 128128. lebedev.ri added a comment. Move `TautologicalUnsignedZeroCompare` and `TautologicalUnsignedEnumZeroCompare` into `TautologicalRangeCompare` too, and enable them only in `-Wextra`. Please advise re flag name for `warn_tautological_constant_compare

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri updated this revision to Diff 128226. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Herald added a subscriber: mehdi_amini. - Give `warn_tautological_constant_compare` it's own flag `-Wtautological-type-limit-compare`. I'm not happy about that name, but i

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri added a comment. Ping. Branching is approaching rapidly. This needs to land before that. Repository: rC Clang https://reviews.llvm.org/D41512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri added a comment. @aaron.ballman, @smeenai, thank you for feedback! Comment at: include/clang/Basic/DiagnosticGroups.td:440 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">; +def TautologicalRangeCompare : DiagGroup<"ta

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri updated this revision to Diff 128462. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Renamed `TautologicalRangeCompare` to `TautologicalInRangeCompare`. Given that previous version of this differential was already previously accepted, and the fact that this n

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

lebedev.ri updated this revision to Diff 128470. lebedev.ri added a comment. Actually update the diagnostic flag :) I'm going to hold off committing just yet, and wait for @rsmith to sign off. Repository: rC Clang https://reviews.llvm.org/D41512 Files: include/clang/Basic/DiagnosticGroups

[PATCH] D41716: clang-tidy: add IgnoreMacros option to readability-inconsistent-declaration-parameter-name

lebedev.ri added a comment. > And also enable it by default to be consistent with e.g. modernize-use-using. That changes the defaults though. I thought clang-tidy *tried* to produce the same results on different clang-tidy versions with the same `.clang-tidy` config? Or is there no such guarant

[PATCH] D41815: [clang-tidy] implement check for goto

lebedev.ri added a comment. In https://reviews.llvm.org/D41815#973260, @JonasToth wrote: > - check that jumps will only be forward. AFAIK that is required in all > sensefull usecases of goto, is it? You could implement loops/recursion with goto, something like: const int end = 10; int i =

[PATCH] D41455: [ASTMatchers] Add isNoReturn() match narrower for FunctionDeclarations

lebedev.ri added a comment. In https://reviews.llvm.org/D41455#963752, @aaron.ballman wrote: > Aside from a documentation nit, this LGTM. However, you should wait to commit > until after you can safely regenerate the AST matcher documentation. The > issue there is that dump_ast_matchers.py was

[PATCH] D41815: [clang-tidy] implement check for goto

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20 + +AST_MATCHER(GotoStmt, isForwardJumping) { + It would be nice to have it in standard ASTMatchers, i believe it will be useful for `else-after-return` check. Though

[PATCH] D43780: [Tooling] [1/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

lebedev.ri added a comment. Thank you. I did re-read the diffs, and i don't see any invalid changes here. I'm going to try to commit this (as two revisions, need to migrate to monorepo one day :/), and if the bots show that i still did miss something here, i'll revert. Repository: rCTE Clang

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