[PATCH] D63954: Add lifetime categories attributes

2019-07-15 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4164 + let Content = [{ +When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function +that returns cv-qualified ``T&`` or ``T*`` is assumed to return a mgehre

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-15 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Please add tests to `llvm/tools/clang/unittests/Tooling/RecursiveASTVisitorTests/`. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332 S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

2019-07-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. In D64448#1581866 , @mgehre wrote: > I didn't know whether they would differ, but the test tells me that they > don't differ significantly (i.e. in introducing new techniques). Okay -- I would prefer if you intentionally

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

2019-07-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/Basic/TokenKinds.def:486 +TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX) +TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX) KEYWORD(__underlying_type , KEYCXX) mgehre wrote: >

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

2019-07-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. In D64448#1581719 , @mgehre wrote: > In D64448#159 , @gribozavr wrote: > > > I don't know how various versions of libstdc++ differ. > > > The current implementation passed the

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

2019-07-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25 + +struct [[gsl::Owner(int)]] OwnerWithConv { + OwnerWithConv(); xazax.hun wrote: > gribozavr wrote: > > Would be nice if the second pair of types had clearly

[PATCH] D63954: Add lifetime categories attributes

2019-07-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4164 + let Content = [{ +When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function +that returns cv-qualified ``T&`` or ``T*`` is assumed to return a Slightly

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

2019-07-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. > Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp. I see. Sorry, but that seems insufficient to me -- different libraries use different patterns. For example, libc++ wraps everything in std in an inline namespace. I don't know how various

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

2019-07-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I'm not an expert in SemaInit code, but this change LGTM. Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:8 + +struct OwnerWithConv; +

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

2019-07-09 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. > The tests are currently here > I think due to their dependency on a standard library, they are not a good > fit for clang/test/. Where else could I put them? Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

2019-07-09 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6549 +static bool +pathInitializeLifetimePointer(llvm::ArrayRef Path) { + return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) { Move closer to the point of usage?

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

2019-07-09 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL365463: Enhance abseil-faster-strsplit-delimiter to handle other non-printable… (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

2019-07-09 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr marked an inline comment as done. gribozavr added inline comments. Comment at: clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:55 + + absl::StrSplit("ABC", R"(A)"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() called with a

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

2019-07-08 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6510 LifetimeBoundCall, +LifetimePointerInit, +LifetimeTempOwner What is this name supposed to mean? Initialization of a "lifetime pointer"? What's a "lifetime pointer"? If

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

2019-07-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Sorry, the patch does not apply cleanly to current master -- could you rebase please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64151/new/ https://reviews.llvm.org/D64151

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

2019-07-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Do you have commit access? Would you like me to commit this patch for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64151/new/

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-07-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL365007: [clang-tidy] new check: bugprone-posix-return (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-07-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Committed revision 365007. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63623/new/ https://reviews.llvm.org/D63623 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

2019-07-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. SVN repo is still there. However, I don't know how to commit using SVN, I commit using the git-svn integration (which still commits using the "svn" command under the hood, but as a user you will be interacting with a git repo). git clone

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

2019-07-02 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Thanks! Do you have commit access? Would you like me to commit this patch for you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63482/new/ https://reviews.llvm.org/D63482

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

2019-07-02 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35 +: FilePath(""), Offset(0), Length(0), ReplacementText("") { + size_t lineBreakPos = ReplacementText.find('\n'); + while (lineBreakPos != std::string::npos) {

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. Thanks! Do you have commit access? Do you want me to commit this patch for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63623/new/ https://reviews.llvm.org/D63623

[PATCH] D63954: Add lifetime categories attributes

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. In D63954#1565128 , @xazax.hun wrote: > In D63954#1565109 , @gribozavr wrote: > > > > I agree. In a follow-up patch we will add the attributes to STL types > > > during parsing. Do you

[PATCH] D63954: Add lifetime categories attributes

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. > I agree. In a follow-up patch we will add the attributes to STL types during > parsing. Do you think it is OK to always add the attributes or should we only > do it if a flag, e.g. -wlifetime is added to the compiler invocation? Warning flags should not change the

[PATCH] D63954: Add lifetime categories attributes

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. > We explicitly allow to add an annotation after the definition of the class to > allow adding annotations to external source from by the user Asking users to forward-declare anything from the standard library is a very bad idea -- in fact it is undefined behavior.

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35 +: FilePath(""), Offset(0), Length(0), ReplacementText("") { + size_t lineBreakPos = ReplacementText.find('\n'); + while (lineBreakPos != std::string::npos) {

[PATCH] D63893: [clang-tidy] Extend TransformerClangTidyCheck to support adding includes.

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:51 + // includes. + if (Rule && std::any_of(Rule->Cases.begin(), Rule->Cases.end(), + [](const

[PATCH] D63892: [LibTooling] Extend `RewriteRule` with support for adding includes.

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63892/new/ https://reviews.llvm.org/D63892 ___ cfe-commits mailing list

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. Good improvements! Comment at: clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp:21 +static StringRef getFunctionSpelling(const MatchFinder::MatchResult , const char *BindingStr) { + const CallExpr

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I'm not an expert in this code, but it looks reasonable. Comment at: clang-tools-extra/clangd/Headers.h:155 + /// \param IncludingFile Used to shorten the include for

[PATCH] D63892: [LibTooling] Extend `RewriteRule` with support for adding includes.

2019-06-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:123 TextGenerator Explanation; +// Include paths that to add to the file affected by

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

2019-06-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/abseil/PrefixedThreadAnnotationsCheck.cpp:43 +FixItHint hint = FixItHint::CreateInsertion(Range.getBegin(), "ABSL_"); +Check.diag(Range.getBegin(), "usage of unprefixed thread annotation") +

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

2019-06-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a reviewer: gribozavr. gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Do you have commit access? Comment at: clang-tools-extra/clang-tidy/abseil/PrefixedThreadAnnotationsCheck.cpp:43 +FixItHint

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-06-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp:61 + if (const auto *AlwaysTrueOp = Result.Nodes.getNodeAs("atop")) { +diag(AlwaysTrueOp->getOperatorLoc(), "%0 returns zero on success and a positive value on failure

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-06-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp:49 +diag(OperatorLoc, "POSIX functions (except posix_openpt) never return negative values") +<< FixItHint::CreateReplacement(OperatorLoc, Twine(">").str()); +

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

2019-06-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:69 + +// A trivial rewrite rule generator that requires ObjectiveC code. +Optional needsObjC(const LangOptions ,

[PATCH] D63784: [clang-tidy] Fix ClangTidyTest to initialize context before checks.

2019-06-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. Good catch! Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h:48 + +template class TestClangTidyAction : public ASTFrontendAction { "CheckTypes"? 'cause "Checks" below is also

[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

2019-06-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp:49 +diag(OperatorLoc, "POSIX functions (except posix_openpt) never return negative values") +<< FixItHint::CreateReplacement(OperatorLoc, Twine(">").str()); +

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

2019-06-25 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst:21 + int ret = posix_fadvise(...); + if (ret != 0) ... why not `if

[PATCH] D63763: [clang-tidy] Update documentation for Qt Creator integration.

2019-06-25 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. LGTM, assuming you know what's new in QtCreator. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63763/new/ https://reviews.llvm.org/D63763

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

2019-06-25 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:92 + const std::string Input = "void log(int);"; + EXPECT_EQ(Input, test::runCheckOnCode(Input)); +} Would

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

2019-06-19 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Thanks! Please add tests in `./unittests/Tooling/ReplacementsYamlTest.cpp`. Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:43 +ReplacementText.replace(lineBreakPos, 1, "\n\n"); +// Get the next occurrence from the current

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:33 +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), Rule(MakeRule(getLangOpts(), Options)) { + assert(llvm::all_of(Rule.Cases,

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:33 +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name,

[PATCH] D63253: [clang-tidy] Made abseil-faster-strsplit-delimiter tests pass on C++17

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363273: [clang-tidy] Made abseil-faster-strsplit-delimiter tests pass on C++17 (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior

[PATCH] D63261: [clang-tidy] Fixed abseil-time-subtraction to work on C++17

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL363272: [clang-tidy] Fixed abseil-time-subtraction to work on C++17 (authored by gribozavr, committed by ). Herald added

[PATCH] D63262: [clang-tidy] Made abseil-upgrade-duration-conversions tests pass on c++17

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363270: [clang-tidy] Made abseil-upgrade-duration-conversions tests pass on c++17 (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D63263: [clang-tidy] Fixed abseil-duration-unnecessary-conversion tests for c++17

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363263: [clang-tidy] Fixed abseil-duration-unnecessary-conversion tests for c++17 (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D63149: Added AST matcher for ignoring elidable constructors

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363262: Added AST matcher for ignoring elidable constructors (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D63261: [clang-tidy] Fixed abseil-time-subtraction to work on C++17

2019-06-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.cpp:45 + match(callExpr(hasParent(varDecl())).bind("e"), + *Node, *Result.Context)) != nullptr; }

[PATCH] D63149: Added AST matcher for ignoring elidable constructors

2019-06-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:69 + CXX17OrLater, + CXX2AOrLater +}; Cxx2aOrLater? (no need to uppercase things) Comment at: clang/unittests/ASTMatchers/ASTMatchersTest.h:159 +

[PATCH] D63149: Added AST matcher for ignoring elidable constructors

2019-06-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:600 + EXPECT_TRUE(matches(code2, matcher2, LanguageMode::CXX11OrLater)); + EXPECT_TRUE(matches(code3, matcher3, LanguageMode::CXX11OrLater)); +} Please inline

[PATCH] D63188: Fixed a crash in misc-redundant-expression ClangTidy checker

2019-06-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363133: Fixed a crash in misc-redundant-expression ClangTidy checker (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D63188: Fixed a crash in misc-redundant-expression ClangTidy checker

2019-06-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr created this revision. gribozavr added a reviewer: ilya-biryukov. Herald added a project: clang. Herald added a subscriber: cfe-commits. It was trying to pass a dependent expression into constant evaluator. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D63188 Files:

[PATCH] D63149: Added AST matcher for ignoring elidable constructors

2019-06-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6455 +/// Matches expressions that match InnerMatcher after any elidable constructor +/// are stripped off. In C++17 copy elidable constructors are no longer being

[PATCH] D63149: Added AST matcher for ignoring elidable constructors

2019-06-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6455 +/// Matches expressions that match InnerMatcher after any elidable constructor are stripped off. +/// 80 columns Comment at:

[PATCH] D63129: [clang-tidy] Fix invalid read on destruction

2019-06-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. This fix works. The alternative would have been to wrap these variables into llvm::ManagedStatic, just like the problematic TrueMatcherInstance in ASTMatchersInternal.cpp. Repository:

[PATCH] D63127: [clang-tidy] Fixed checker for abseil to work in C++17 mode

2019-06-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:86 +.bind("ByAnyChar"), +expr(hasDescendant(cxxConstructExpr( + hasType(recordDecl(hasName("::absl::ByAnyChar"))),

[PATCH] D63128: Fixed google-readability-casting test to work in c++17

2019-06-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL363047: Fixed google-readability-casting test to work in c++17 (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D63128: Fixed google-readability-casting test to work in c++17

2019-06-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I'll commit this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63128/new/ https://reviews.llvm.org/D63128

[PATCH] D61486: [Basic] Introduce active dummy DiagnosticBuilder

2019-06-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Is this patch still needed? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61486/new/ https://reviews.llvm.org/D61486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr marked an inline comment as done. gribozavr added inline comments. Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:20 + pipe2(pipefd, O_NONBLOCK); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'pipe2' should use O_CLOEXEC where possible

[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Looks great, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62845/new/ https://reviews.llvm.org/D62845

[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. In D62845#1529149 , @hokein wrote: > > I think we should be looking at the intent of the test rather than its name. > > > > The intent looks like testing how the check works when `std::make_unique` > > is available from the

[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. In D62845#1528887 , @hokein wrote: > In D62845#1528791 , @gribozavr wrote: > > > I'd suggest to add a separate file that covers the exact language modes > > needed. > > > > The C++14

[PATCH] D62045: Revise the google-objc-global-variable-declaration check to match the style guide.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Sorry for jumping in late, but renaming the declaration is not enough -- usages should also be updated; otherwise the developer is better off using a refactoring in their IDE or even a textual search and replace. Repository: rL LLVM CHANGES SINCE LAST ACTION

[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. I'd suggest to add a separate file that covers the exact language modes needed. The C++14 test that we have right now is about C++14-or-later, testing the availability of std::make_unique. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D62845: [clang-tidy] Fix make-unique tests on C++2a.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. I'm also not sure what the intent behind these tests is. Maybe the right fix is to add a constructor that can be called? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62845/new/ https://reviews.llvm.org/D62845

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:20 + pipe2(pipefd, O_NONBLOCK); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'pipe2' should use O_CLOEXEC where possible [android-cloexec-pipe2] + // CHECK-FIXES:

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

2019-06-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:14 +int pipe(int pipefd[2]); +void noWarning() { + int pipefd[2]; noWarningForPipeInNamespace ? Comment

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

2019-06-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472 if (Info.hasSourceManager()) checkFilters(Info.getLocation(), Info.getSourceManager()); } nik wrote: > gribozavr wrote: > > It seems like the `checkFilters` call

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

2019-06-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:5 + +void f() { + int pipefd[2]; Please give the tests informative names

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:306 +if (CEArg->isElidable()) { + if (const auto *TempExp = CEArg->getArg(0)) { +if (const auto *UnwrappedCE = Eugene.Zelenko

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:306 +if (CEArg->isElidable()) { + if (const auto *TempExp = CEArg->getArg(0)) { +if (const auto *UnwrappedCE = Eugene.Zelenko

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:54 + +void e() { + int pipefd[2]; srhines wrote: > gribozavr wrote: > > How is `e` different from `a`? > `e` uses O_CLOEXEC properly with `pipe2()` and

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:306 +if (CEArg->isElidable()) { + if (const auto *TempExp = CEArg->getArg(0)) { +if (const auto *UnwrappedCE = Eugene.Zelenko

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:317 + + if (pipe(InotifyPollingStopperFDs) == -1) +return nullptr; Use pipe2() with O_CLOEXEC, to avoid leaking the file descriptors to child

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:40 +// FDRead. +// Currently used just for one-off termination signal. +struct SemaphorPipe

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr marked an inline comment as done. gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:306 +if (CEArg->isElidable()) { + if (const auto *TempExp = CEArg->getArg(0)) { +if (const auto

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr requested changes to this revision. gribozavr added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst:6 + +Checks if the required file flag ``O_CLOEXEC`` is present in the

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:31 + Result, + "prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC", + ReplacementText); hokein wrote: > the message doesn't seem to

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. Very nice testing approach! Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:20 +/// Provides notifications for file changes in a directory. + +/// Invokes client-provided function on every filesystem event in the watched

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:311 + } +} +if (CEArg->isStdInitListInitialization()) So `Foo(Bar{1, 2})` is not problematic, it is just that we can't tell it

[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

2019-05-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:302 + if (const auto *CEArg = dyn_cast(Arg)) { +// Strip the ediable move constructor, it is presented in C++11/14 mode. +// e.g. Foo(Bar{1, 2}), the

[PATCH] D62616: [CodeComplete] Add a bit more whitespace to completed patterns

2019-05-29 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I totally agree about the space before the opening curly, but the space before the opening paren is a matter of style. Certainly it does match LLVM style better, but it does not match

[PATCH] D62524: [clang-tidy] Fix description for misc-definitions-in-headers.

2019-05-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp:133 +<< IsFullSpec << FD; +diag(FD->getLocation(), /*FixDescription=*/"make

[PATCH] D61350: [clang-tidy] New check calling out uses of +new in Objective-C code

2019-05-23 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rCTE361487: [clang-tidy] New check calling out uses of +new in Objective-C code (authored by gribozavr, committed by ). Changed prior to commit: https://reviews.llvm.org/D61350?vs=200824=200931#toc

[PATCH] D62187: Delete default constructors, copy constructors, move constructors, copy assignment, move assignment operators on Expr, Stmt and Decl

2019-05-23 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. gribozavr marked an inline comment as done. Closed by commit rC361468: Delete default constructors, copy constructors, move constructors, copy… (authored by gribozavr, committed by ). Changed prior to commit:

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

2019-05-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:457 + forwardDiagnostic(Info); + return; + } Indentation is 2 spaces. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472 if

[PATCH] D62187: Delete default constructors, copy constructors, move constructors, copy assignment, move assignment operators on Expr, Stmt and Decl

2019-05-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr marked 2 inline comments as done. gribozavr added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1057 + Stmt() = delete; Stmt(const Stmt &) = delete; ilya-biryukov wrote: > gribozavr wrote: > > ilya-biryukov wrote: > > > NIT: Move

[PATCH] D62187: Delete default constructors, copy constructors, move constructors, copy assignment, move assignment operators on Expr, Stmt and Decl

2019-05-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr updated this revision to Diff 200684. gribozavr marked 2 inline comments as done. gribozavr added a comment. Addressed review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62187/new/ https://reviews.llvm.org/D62187 Files:

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 

2019-05-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added a comment. > I prefer Option 2 because it is a cleaner, more understandable design for the > matchers. I agree with Aaron. Clang or Clang Tooling provide no promise of source stability. Of course we should not break things just because we can, but API coherence and

[PATCH] D62187: Delete default constructors, copy constructors, move constructors, copy assignment, move assignment operators on Expr, Stmt and Decl

2019-05-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr marked 4 inline comments as done. gribozavr added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:374 + Decl(const Decl&) = delete; + Decl(Decl &&) = delete; + Decl =(const Decl&) = delete; ilya-biryukov wrote: > NIT: move

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

2019-05-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:32 +// }; +class TransformerTidy : public ClangTidyCheck { +public: ymandel wrote: > gribozavr wrote: > > I'd prefer a name like "TransformerClangTidyCheck", it

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136 + +const int PollResult = poll(, 1, TimeoutMs); +// There are inotify events waiting to be read! jkorous wrote: > gribozavr

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

2019-05-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/utils/TransformerTidy.h:32 +// }; +class TransformerTidy : public ClangTidyCheck { +public: I'd prefer a

[PATCH] D62187: Delete default constructors, copy constructors, move constructors, copy assignment, move assignment operators on Expr, Stmt and Decl

2019-05-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr created this revision. gribozavr added reviewers: ilya-biryukov, rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D62187 Files: clang/include/clang/AST/DeclBase.h

[PATCH] D62150: Renamed `apply` to `select` to avoid ADL conflict with `std::apply`

2019-05-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rC361170: Renamed `apply` to `select` to avoid ADL conflict with `std::apply` (authored by gribozavr, committed by ). Changed prior to commit: https://reviews.llvm.org/D62150?vs=200313=200316#toc

[PATCH] D62150: Renamed `apply` to `select` to avoid ADL conflict with `std::apply`

2019-05-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr created this revision. gribozavr added a reviewer: ilya-biryukov. Herald added a project: clang. Herald added a subscriber: cfe-commits. `std::apply` in C++14 and above is defined with two unrestricted arguments, and it wins overload resolution in this case. Repository: rG LLVM

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

2019-05-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr added inline comments. Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:78 + /// The DirectoryWatcher that originated this event is no longer valid and + /// it's behavior is undefined. + /// The prime case is kernel signalling to

  1   2   3   >