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

lebedev.ri reopened this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. And reverted. This broke gcc4.8 builds, compiler just segfaults: http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/14909 ... [72/368] Building CXX object tools/c

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Tried fixing `tooling::FrontendActionFactory::create()` in https://reviews.llvm.org/D43779/https://reviews.llvm.org/D43780, but had to revert due to gcc4.8 issues :/ Thank you for working on this, some more review notes. In https://reviews.llvm.org/D41102#1020107, @

[PATCH] D43829: [Sema] Add -Wparentheses warnings for macros (PR 18971)

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

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

lebedev.ri added a comment. In https://reviews.llvm.org/D43779#1021959, @alexfh wrote: > Do you have a way to reproduce the gcc crashes? Not presently. I'm on debian sid, so gcc4.8 is a lost-for-good relic from ancient past. I'll try to create an debian-oldstable chroot, which still has gcc4.8

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/BitcodeWriter.h:37 + static constexpr unsigned SubblockIDSize = 4U; + static constexpr unsigned BoolSize = 1U; + static constexpr unsigned IntSize = 16U; Hmm, you build with asserts enabled, right? I trie

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

lebedev.ri added a comment. Thank you for the feedback! In https://reviews.llvm.org/D43162#1022427, @rsmith wrote: > This is the wrong way to deal with this. The only thing that should ever be > controlled by -W flags is whether the warnings in that group appear, not > whether warnings in othe

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Thank you for working on this! Some more review notes. Please look into adding a bit more tests. Comment at: clang-doc/BitcodeWriter.cpp:179 + assert(Inits.size() == RecordIdCount); + for (const auto &Init : Inits) RecordIdNameMap[Init.firs

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

lebedev.ri updated this revision to Diff 136503. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Reworked stuff via new `CXX98CompatExtraSemi` diag group. As expected, passing `-Wno-c++98-compat-pedantic` disables the extra-semi diag, which exactly the opposite from w

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

lebedev.ri added a comment. **If** i'm reading `git blame` correctly, the `-Wnewline-eof` diag, which i used as a base to model the previous version of this diff on, was added in https://reviews.llvm.org/rL189110 / https://github.com/llvm-mirror/clang/commit/7865b8e4324378e06f59adb4d60bec26a7d3d

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Thank you for working on this! Some more nitpicking. //Please// consider adding even more tests (ideally, all this code should have 100% test coverage) Comment at: clang-doc/BitcodeWriter.cpp:139 + {COMMENT_NAME, {"Name", &StringAbbrev}},

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Could some other people please review this differential, too? I'm sure i have missed things. --- Some more nitpicking. For this differential as standalone, i'we mostly run out of things to nitpick. Some things can probably be done better (the blockid/recordid stuff c

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/Representation.h:117 + bool IsDefinition = false; + llvm::Optional DefLoc; + llvm::SmallVector Loc; I meant that `IsDefinition` controls whether `DefLoc` will be set/used or not. So with `llvm::Optional D

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

lebedev.ri added a comment. In https://reviews.llvm.org/D40737#1024120, @JonasToth wrote: > After long inactivity (sorry!) i had a chance to look at it again: > > switch(i) { > case 0:; > case 1:; > case 2:; > ... > } > > > does *NOT* lead to the stack overflow. This is most likely an

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

lebedev.ri added a comment. Thank you for working on this! Finally trying to review this... I must say i'm **really** not fond of the test 'changes'. But some initial comments added: Comment at: clang-doc/BitcodeReader.cpp:19 + +void ClangDocBitcodeReader::storeData(llvm::Sma

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Nice! Some further notes based on the SHA1 nature. Comment at: clang-doc/BitcodeWriter.cpp:74 + AbbrevGen(Abbrev, +{// 0. Fixed-size integer (ref type) + llvm::BitCodeAbbrevOp(llvm::BitCodeAbbrevOp::Fixed, Th

[PATCH] D44173: [clang-tidy] Add "portability" module and rename readability-simd-intrinsics to portability-simd-intrinsics

lebedev.ri added a comment. I should have seen in during the initial review, but better now than never, right? :) Comment at: clang-tidy/portability/SIMDIntrinsicsCheck.cpp:141 if (!New.empty()) { std::string Message; // If Suggest is true, give a P0214 alternativ

[PATCH] D44093: [BUILTINS] structure pretty printer

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

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. In https://reviews.llvm.org/D41102#1028995, @lebedev.ri wrote: > Some further notes based on the SHA1 nature. I'm sorry, brainfreeze, i meant `40` chars, not `20`. Updated comments... Comment at: clang-doc/BitcodeWriter.cpp:309 + assert(Ref.USR.s

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Hmm, i'm missing something about the way store sha1... Comment at: clang-doc/BitcodeWriter.cpp:53 +{// 0. Fixed-size integer (length of the sha1'd USR) + llvm::BitCodeAbbrevOp(llvm::BitCodeAbbrevOp::VBR, +

[PATCH] D44241: Add a check for constructing non-trivial types without assigning to a variable

lebedev.ri added a comment. I'd like to see some more tests void valid() { g(NonTrivial{}); // Should not warn } Have you already tried running it on some C++ codebase (llvm, qt, boost, ...) ? What are the results? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44241

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

lebedev.ri added a comment. Please do bother to prefix with the appropriate `[prefix]`, set the appropriate repo, and tags! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#1021863, @chandlerc wrote: > In https://reviews.llvm.org/D36836#931995, @lebedev.ri wrote: > > > - Rebased > > - As advised by @aaron.ballman, moved into it's own directory/module. > > Please review that, i'm not entirely sure i have

[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] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Might have been better to not start landing until the all differentials are understood/accepted, but i understand that it is not really up to me to decide. Let's hope nothing in the next differentials will require changes to this initial code :) Co

[PATCH] D16008: [clang-tidy] Add calling virtual functions in constructors/destructors check.

lebedev.ri added a comment. Herald added subscribers: llvm-commits, xazax.hun, mgorny, klimek. Any further thoughts here? I was slightly bitten by this recently, and i though that it already existed as a clang-tidy check (: Repository: rL LLVM https://reviews.llvm.org/D16008 _

[PATCH] D44093: [BUILTINS] structure pretty printer

lebedev.ri added a comment. BTW, as far as i can tell this still has zero test coverage (no new tests are being added). I'd expect to see the tests for the actual output - one struct per each type it is able to print - probably some tests showing error handling, and possibly the availability of

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Since the commit was reverted, did you mean to either recommit it, or reopen this (with updated diff), so it does not get lost? Repository: rL LLVM https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-comm

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

lebedev.ri added a comment. While i don't have a leg to stand on here, i'd be *much* more comfortable if this would be a proper RFC mail in cfe-dev, that would explore all the possible options (this, and the one from cfe-dev thread clang-tidy and CppCoreGuidelines

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

lebedev.ri added a comment. Once the base differential firmly lands, could you please rebase this so the review could continue? https://reviews.llvm.org/D43341 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

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

lebedev.ri added a comment. Let's move weekly ping away from friday :) 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] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

lebedev.ri added inline comments. Comment at: test/SemaCXX/extra-semi.cpp:14 +void F(){} +; // expected-warning {{extra ';' outside of a function is}} + aaron.ballman wrote: > Can you use the full diagnostic text for this first appearance in the test? Will requir

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added inline comments. Comment at: clang-doc/BitcodeWriter.cpp:230 + prepRecordData(ID); + for (const char C : RecordIdNameMap[ID].Name) Record.push_back(C); + Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_SETRECORDNAME, Record); lebedev.ri wrote: >

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

lebedev.ri added a comment. In https://reviews.llvm.org/D43322#1037889, @Quuxplusone wrote: > The backward-compatibility-concerned diagnostic, > `-Wreturn-std-move-in-c++11`, is *not* appropriate for `-Wmove`; Have you evaluated possibility of adding `-Wreturn-std-move-in-c++11` to one of `CX

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. So what part is failing, specifically? The SHA1 blobs of USR's differ in the llvm-bcanalyzer dumps? The actual filenames `%t/docs/bc/` differ? I guess both? First one you should be able to handle by replacing the actual values with a regex (i'd guess ` op19=226 op20=2

[PATCH] D44559: [Sema] Wrong width of result of mul operation

lebedev.ri added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8541 + switch (C.getTargetInfo().getTriple().getArch()) { + case llvm::Triple::x86: + case llvm::Triple::x86_64: { I agree with @RKSimon, i don't see why this would be arch-spe

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

lebedev.ri added a comment. Why not `test/SemaObjC/block_compare.mm` ? Repository: rC Clang https://reviews.llvm.org/D44580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44559: [Sema] Wrong width of result of mul operation

lebedev.ri added a comment. In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > I think we're correct not to warn here and that GCC/ICC are being noisy. The > existence of a temporary promotion to a wider type doesn't justify warning on > arithmetic between two operands that are the

[PATCH] D44559: [Sema] Wrong width of result of mul operation

lebedev.ri added a comment. In https://reviews.llvm.org/D44559#1041001, @rjmccall wrote: > In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > > > > > I think we're correct not to warn here and that GCC/ICC are being

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

lebedev.ri created this revision. lebedev.ri added reviewers: alexfh, hokein, xazax.hun, JonasToth, aaron.ballman. lebedev.ri added a project: clang-tools-extra. Herald added a subscriber: rnkovacs. Pretty straight-forward, just count all the variable declarations in the function's body, and if m

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

lebedev.ri updated this revision to Diff 138830. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Adjusted ReleaseNotes change. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44602 Files: clang-tidy/readability/FunctionSizeCheck.cpp clang-tidy/readab

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Hm, or possibly you could just pass the triple to clang? Repository: rL LLVM https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

lebedev.ri updated this revision to Diff 138844. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Address jonastoth's review notes. - Properly support C++17 structured bindings. - A few more tests Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44602 Fil

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

lebedev.ri added a comment. In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote: > Looks fine to me, but Aaron or someone else must accept :) Thanks for review! In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote: > Just out of curiosity: The decomposition visit is for struc

[PATCH] D44607: Recompute invalidated iterator in insertTargetAndModeArgs

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

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

lebedev.ri added a comment. Please always upload all patches with full context (`-U99`) Repository: rC Clang https://reviews.llvm.org/D44609 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

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

[PATCH] D44155: Fix support for try_acquire_capability

lebedev.ri added a comment. Please upload patches with full context (`-U9`) :) https://reviews.llvm.org/D44155 ___ 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 a comment. In https://reviews.llvm.org/D41102#1041773, @juliehockett wrote: > I was just thinking of disabling the one test that has an issue > (class-in-function) on Windows -- the filename is only used in generating > *some* USRs, so all of the other ones are fine. We ran int

[PATCH] D41102: Setup clang-doc frontend framework

lebedev.ri added a comment. Huh, something weird is going on there. What about the other way around, `REQUIRES: linux` ? Repository: rL LLVM https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

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

lebedev.ri added a comment. I would like to see more negative tests. What does it do if the len/size is a constant? Variable that wasn't just assigned with `strlen()` return? https://reviews.llvm.org/D45050 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files

lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1116973, @george.karpenkov wrote: > LGTM with a nit to me Thank you! I suspect that at least temporarily we will end up with two different tooling sets to further post-process these results, since i really love to write bicycles a

[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:282-290 +def warn_unused_using_declaration : Warning< + "unused using declaration %0">, + InGroup, DefaultIgnore; +def warn_unused_using_directive : Warning< + "unused using directive %0">

[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files

lebedev.ri updated this revision to Diff 149610. lebedev.ri marked 6 inline comments as done. lebedev.ri added a comment. Rebased. Addressed review notes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 Files: clang-tidy/ClangTidy.cpp clang-tidy/ClangTidy.h clang-tid

[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files

lebedev.ri added inline comments. Comment at: clang-tidy/tool/ClangTidyMain.cpp:342-347 +if (!llvm::sys::fs::exists(AbsolutePath)) { + // If the destination prefix does not exist, don't try to use real_path(). + return AbsolutePath; +} +SmallString<256> des

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

lebedev.ri added a comment. In https://reviews.llvm.org/D45050#1119973, @Charusso wrote: > In https://reviews.llvm.org/D45050#1116446, @lebedev.ri wrote: > > > I would like to see more negative tests. > > What does it do if the len/size is a constant? > > Variable that wasn't just assigned with

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added reviewers: rsmith, aaron.ballman. lebedev.ri added a comment. Tests? Repository: rC Clang https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added a comment. In https://reviews.llvm.org/D47687#1120213, @Higuoxing wrote: > As for some test cases, The tests need to be within the patch itself, in this case i guess that should go into `clang/test/Sema/`. See other tests in there on how to write them. And they will be run via

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added subscribers: dblaikie, rnk. lebedev.ri added a comment. In https://reviews.llvm.org/D47687#1120971, @Higuoxing wrote: > Update with test cases :) Thanks. I do believe we are being overly forgiving for macros. Sometimes that makes sense, sometimes not, especially there is no pr

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added reviewers: rjmccall, akyrtzi. lebedev.ri added a comment. It seems it was https://reviews.llvm.org/rL119537 that regressed it. Nice, such a change, no review, no test :) https://reviews.llvm.org/D47687 ___ cfe-commits mailing list c

[PATCH] D46602: [clang-tidy] Store checks profiling info as JSON files

lebedev.ri added inline comments. Comment at: clang-tidy/ClangTidyProfiling.cpp:47 +void ClangTidyProfiling::printAsJSON(llvm::raw_ostream &OS) { + OS << "{\n"; aaron.ballman wrote: > I'm not keen that we call this `printAsJSON()` when the docs talk about > w

[PATCH] D46602: [clang-tidy] Store checks profiling info as JSON files

lebedev.ri updated this revision to Diff 149933. lebedev.ri marked 5 inline comments as done. lebedev.ri added a comment. Rebase ontop of https://reviews.llvm.org/rL333994, address review notes, canonicalize to `JSON`. Repository: rL LLVM https://reviews.llvm.org/D46602 Files: clang-tidy/

[PATCH] D46602: [clang-tidy] Store checks profiling info as JSON files

lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1122194, @aaron.ballman wrote: > LGTM! Thank you for the review. Waiting on @alexfh ... Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commit

[PATCH] D46602: [clang-tidy] Store checks profiling info as JSON files

lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1123631, @alexfh wrote: > LG Thank you for the review! Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added a comment. In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote: > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > @dexonsmith is there someone from Apple who can comment on rdar://8678458 > > and the merits of disabling this warning in macros? I strongly s

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added a comment. + there will need to be a new standalone test for `-Wlogical-op-parentheses-in-macros`, what it does, how it relates to `-Wlogical-op-parentheses`,`-Wparentheses`, etc. Comment at: include/clang/Basic/DiagnosticGroups.td:264-265 def LogicalOpParent

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:264-265 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; +def LogicalOpParenthesesInMacros: DiagGroup<"logical-op-parentheses-in-macros">; def LogicalNotParentheses: DiagGroup<"

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:264-265 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; +def LogicalOpParenthesesInMacros: DiagGroup<"logical-op-parentheses-in-macros">; def LogicalNotParentheses: DiagGroup<"

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

lebedev.ri added inline comments. Comment at: test/Sema/logical-op-parentheses-in-macros.c:3-4 +// RUN:-verify=logical-op-parentheses %s +// RUN: %clang_cc1 -fsyntax-only %s +// RUN: %clang_cc1 -Wparentheses -fsyntax-only %s + The comment got lost, b

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

lebedev.ri added a comment. In https://reviews.llvm.org/D45050#1127449, @Charusso wrote: > In https://reviews.llvm.org/D45050#1119974, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D45050#1119973, @Charusso wrote: > > > > > In https://reviews.llvm.org/D45050#1116446, @lebedev.ri wrote: > >

[PATCH] D47435: Add __builtin_signbit semantic checking

lebedev.ri added a comment. The checking by itself looks sane-ish, but i don't have any reasonable knowledge in this area. Comment at: lib/Sema/SemaChecking.cpp:925 + case Builtin::BI__builtin_signbitl: if (SemaBuiltinFPClassification(TheCall, 1)) return ExprError

[PATCH] D47435: Add __builtin_signbit semantic checking

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Looks good to me, but you probably want a bit for a second opinion. https://reviews.llvm.org/D47435 ___ cfe-commits mailing list cfe-comm

[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.

lebedev.ri added a comment. In https://reviews.llvm.org/D45679#1122826, @shuaiwang wrote: > Thanks a lot for the review! > > Could you also help commit this diff as well? Thanks! There are at least two previous accepted&committed differentials (https://reviews.llvm.org/D17586, https://reviews.

[PATCH] D48134: [CodeGen] make nan builtins pure rather than const (PR37778)

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Makes sense. https://reviews.llvm.org/D48134 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[PATCH] D48134: [CodeGen] make nan builtins pure rather than const (PR37778)

lebedev.ri added a comment. In https://reviews.llvm.org/D48134#1131626, @rsmith wrote: > Can we mark these as `argmemonly`? Header comment in `include/clang/Basic/Builtins.def` does not list that as a possibility. Repository: rL LLVM https://reviews.llvm.org/D48134

[PATCH] D48098: clang-format-diff: Switch to python3 by default, support python 2.7

lebedev.ri added a comment. Please always upload all patches with the full context (`-U9`) Comment at: tools/clang-format/clang-format-diff.py:1 -#!/usr/bin/env python +#!/usr/bin/env python3 # Why do you need to *switch* the default? What's wrong with [at

[PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

lebedev.ri added a comment. This seems to be missing tests. Repository: rC Clang https://reviews.llvm.org/D47953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

lebedev.ri updated this revision to Diff 152587. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Herald added a subscriber: llvm-commits. Right, sorry. Let's try to revive this. Does this test coverage look better? What did i miss? Repository: rL LLVM https://reviews.

[PATCH] D53042: [X86] Remove FeatureRTM from Skylake processor list

lebedev.ri added inline comments. Comment at: lib/Basic/Targets/X86.cpp:169 setFeatureEnabledImpl(Features, "mpx", true); if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX setFeatureEnabledImpl(Features, "sgx", true); > h

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

lebedev.ri added a comment. In https://reviews.llvm.org/D52998#1258962, @eandrews wrote: > Yes. I understand. At the moment, exception handling flags are generated > based on `BENCHMARK_ENABLE_EXCEPTIONS` in `utils/benchmark/CMakeLists.txt` . > However, `_HAS_EXCEPTIONS` is not defined based

[PATCH] D52727: [clang-tidy] White List Option for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. LG other than the `hasCanonicalType()` vs `hasType()` question. https://reviews.llvm.org/D52727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53042: [X86] Remove FeatureRTM from Skylake processor list

lebedev.ri added inline comments. Comment at: lib/Basic/Targets/X86.cpp:169 setFeatureEnabledImpl(Features, "mpx", true); if (Kind != CK_SkylakeServer) // SKX inherits all SKL features, except SGX setFeatureEnabledImpl(Features, "sgx", true); thi

[PATCH] D52727: [clang-tidy] White List Option for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a reviewer: JonasToth. lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LG unless @JonasToth or @aaron.ballman has any further comments. https://reviews.llvm.org/D52727

[PATCH] D50766: Fix false positive unsequenced access and modification warning in array subscript expression.

lebedev.ri added inline comments. Comment at: test/SemaCXX/warn-unsequenced-cxx17.cpp:1 +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wno-unused %s + One last-minute thought: this is only a positive test. You don't test what happens before C++17. https:

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri added a comment. In https://reviews.llvm.org/D50901#1261203, @rsmith wrote: > This looks good to me. Sounds like @filcab is intending on doing another > round of review too, so it'd make sense to double-check there before > committing. In https://reviews.llvm.org/D50901#1261312,

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

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

[PATCH] D50766: Fix false positive unsequenced access and modification warning in array subscript expression.

lebedev.ri added inline comments. Comment at: test/SemaCXX/warn-unsequenced-cxx17.cpp:1 +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wno-unused %s + Rakete wrote: > lebedev.ri wrote: > > One last-minute thought: this is only a positive test. > > You

[PATCH] D52839: Inform AST's UnaryOperator of FENV_ACCESS

lebedev.ri added inline comments. Comment at: include/clang/AST/Expr.h:1791 + // otherwise. + unsigned FPFeatures : 3; + Why as the first one, and not after `CanOverflow` ... Comment at: include/clang/AST/Expr.h:1809 +Opc(opc), CanOve

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

lebedev.ri added a comment. In https://reviews.llvm.org/D52835#1263032, @xbolva00 wrote: > As noted, this in under LiteralConversion (-Wconversion). GCC has it too > under -Wconversion, so I think it is fine as is, or? > @rsmith It's not so much about "which flag group do i need to enable to

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri updated this revision to Diff 169401. lebedev.ri marked 2 inline comments as done. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Let's try to wrap this up. - Dropped HICPP alias. I/we really don't understand what those guidelines require. - Don't touch un

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri added inline comments. Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:56 + auto RecordIsInteresting = + allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(), +IgnoreClassesWithAllMemberVariablesBeingPublic --

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri updated this revision to Diff 169405. lebedev.ri added a comment. Dead code elimination. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52771 Files: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tidy/misc/CMakeLists.txt clang-tidy/misc/Misc

[PATCH] D52839: Inform AST's UnaryOperator of FENV_ACCESS

lebedev.ri added a comment. I think you have uploaded the wrong patch here. https://reviews.llvm.org/D52839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: docs/ReleaseNotes.rst:113 + +- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes + ` Eugene.Zelenko wrote: > Please move new alias after new check

[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)

lebedev.ri updated this revision to Diff 169554. lebedev.ri marked 8 inline comments as done. lebedev.ri added a comment. Hopefully address review notes: - Support more (all?) obscure suffixes (complex, q, h, f16, i{8,16,32,64}) - Improve test coverage. There isn't coverage for every single comb

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri updated this revision to Diff 169555. lebedev.ri added a comment. Sort release notes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52771 Files: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTid

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

lebedev.ri added inline comments. Comment at: docs/ReleaseNotes.rst:113 + +- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes + ` JonasToth wrote: > lebedev.ri wrote: > > Eugene.Zelenko wrote: > > > Please move new alias after new check

[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)

lebedev.ri updated this revision to Diff 169556. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Bloat with `llvm::find_if()` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52670 Files: clang-tidy/cert/CERTTidyModule.cpp clang-tidy/cert/CMakeLists.t

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

lebedev.ri added a comment. Ping. The prerequisite "split truncation sanitizer into unsigned and signed cases" has landed. I believe i have replied/addressed all the points previously raised here. Would be awesome to get this going at long last :) Repository: rC Clang https://reviews.llvm.or

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

lebedev.ri added a comment. I think it would be good to add some more explanation as to *why* that `else` has to be kept. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

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

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