[PATCH] D66627: [clang-tidy] add checks to bugprone-posix-return

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. LGTM with all comments addressed. Comment at: clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp:169 int posix_fadvise(int fd, off_t offset, off_t len, int ad

[PATCH] D66788: Refactor GlobList from an ad-hoc linked list to a vector

gribozavr marked an inline comment as done. gribozavr added inline comments. Comment at: clang-tools-extra/clang-tidy/GlobList.cpp:46 +GlobList::GlobList(StringRef Globs) { + do { +GlobListItem Item; ilya-biryukov wrote: > NIT: I suggest using `for (;!Globs.

[PATCH] D66787: GlobList: added a clear test for pattern priority

This revision was automatically updated to reflect the committed changes. Closed by commit rGfd2315ce2101: GlobList: added a clear test for pattern priority (authored by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66787/new/ https://re

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Frontend/CompilerInvocation.h:150 /// + /// If an error was encountered while parsing the arguments, \returns false + /// and a

[PATCH] D66788: Refactor GlobList from an ad-hoc linked list to a vector

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. I think it makes method implementations more obvious. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66788 Files: clang-tools-extra/clang-tidy/GlobList.cpp clang-tools-e

[PATCH] D66787: GlobList: added a clear test for pattern priority

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. gribozavr added a reviewer: ilya-biryukov. The last glob that matches the string decides whether that string is included or excluded. Repository: rG LLVM Github Monorepo https://reviews.llv

[PATCH] D66694: [libclang][index][NFCi] Refactor machinery for skipping already parsed function bodies

gribozavr added inline comments. Comment at: clang/tools/libclang/Indexing.cpp:126 +/// Is thread-safe. +class SharedParsedRegionsStorage { std::mutex Mux; "SharedParsedRegions"? "ThreadSafeParsedRegions"? Comment at: clang/tools/libclang/In

[PATCH] D66747: Moved GlobList into a separate header file

This revision was automatically updated to reflect the committed changes. Closed by commit rGa6fed93f0d10: Moved GlobList into a separate header file (authored by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66747/new/ https://reviews.l

[PATCH] D66747: Moved GlobList into a separate header file

gribozavr created this revision. Herald added subscribers: cfe-commits, mgorny. Herald added a project: clang. It is a separate abstraction that is used in more contexts than just a helper for ClangTidyDiagnosticConsumer. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66747 Fi

[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. I see, it all makes sense now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66676/new/ https://reviews.llvm.org/D66676 ___

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

gribozavr added a comment. Sorry for the silly comments, but my main point is, I guess, that I don't quite understand the design towards which you are refactoring, and to meaningfully review patches I need to be on the same page. Comment at: clang/include/clang/StaticAnalyzer

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6775 +if (!pathOnlyInitializesGslPointer(Path)) + Init = const_cast(Init->skipRValueSubobjectAdjustments()); xazax.hun wrote: > gribozavr wrote: > > I'm afraid this change could d

[PATCH] D66676: [clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.

gribozavr added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:106 +// argument, while the change spans only the argument AND there are two such +// matches. Here, we expect a conflict between the two matches and the second

[PATCH] D66731: [Driver] Add an option for createInvocationFromCommandLine to recover on errors

gribozavr added a comment. It does seem reasonable for Clangd to recover when it sees unknown command line flags (after all, in compiler_commands.json we can see command line flags from past and future versions of Clang, GCC and other compilers -- whatever the build system decided to write ther

[PATCH] D66627: [clang-tidy] new check: bugprone-pthread-return

gribozavr added a comment. Thanks for the contribution! In abstract, I think it is a good checker, however, the implementation largely duplicates `clang-tidy/bugprone/PosixReturnCheck.cpp` -- do you think you could factor out the common parts? I see at least two ways: - Factor out a library, u

[PATCH] D66722: [clang] Ensure that comment classes are trivially destructible

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Regarding adding a test for `DeclInfo` -- SGTM as well. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66722/new/ https://reviews.llvm.org/D66722 _

[PATCH] D66706: [Wdocumentation] fixes an assertion failure with typedefed function and block pointer

gribozavr added inline comments. Comment at: clang/lib/AST/Comment.cpp:151 +static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL, + bool testTypedefTypeLoc = false) { TypeLoc PrevTL; Mordante wrote: > gribozavr wrote

[PATCH] D66700: [Wdocumentation] improve wording of a warning message

This revision was automatically updated to reflect the committed changes. Closed by commit rL369873: [Wdocumentation] improve wording of a warning message (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: http

[PATCH] D66706: [Wdocumentation] fixes an assertion failure with typedefed function and block pointer

gribozavr added inline comments. Comment at: clang/lib/AST/Comment.cpp:151 +static bool getFunctionTypeLoc(TypeLoc TL, FunctionTypeLoc &ResFTL, + bool testTypedefTypeLoc = false) { TypeLoc PrevTL; Why is the new functionality turn

[PATCH] D66700: [Wdocumentation] improve wording of a warning message

gribozavr added a comment. I will commit on Monday. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66700/new/ https://reviews.llvm.org/D66700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D66652: [libTooling] Transformer: refine `SourceLocation` specified as anchor of changes.

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:197 // Verify the existence and validity of the AST node that roots this rule. + SourceLocation Root

[PATCH] D66631: [clang-tidy] Don't emit google-runtime-references warning for functions defined in macros.

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. LGTM, although I'd be more comfortable with a whitelist of macros. Comment at: clang-tools-extra/test/clang-tidy/google-runtime-references.cpp:156 +DEFINE_F(func) {} \

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4596 +for (Decl *Redecl : D->redecls()) { + Redecl->addAttr(::new (S.Context) + OwnerA

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6775 +if (!pathOnlyInitializesGslPointer(Path)) + Init = const_cast(Init->skipRValueSubobjectAdjustments()); I'm afraid this change could disable some other analysis, which would

[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files

This revision was automatically updated to reflect the committed changes. gribozavr marked an inline comment as done. Closed by commit rL369504: Removed some dead code in BugReporter and related files (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber:

[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files

gribozavr marked 2 inline comments as done. gribozavr added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2343 InterExplodedGraphMap ForwardMap; - TrimmedGraph = OriginalGraph->trim(Nodes, &ForwardMap, &InverseMap); Szelethus wr

[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files

gribozavr updated this revision to Diff 216342. gribozavr marked an inline comment as done. gribozavr added a comment. Updated a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66473/new/ https://reviews.llvm.org/D66473 Files: clang/inclu

[PATCH] D66473: [analyzer] Removed some dead code in BugReporter and related files

gribozavr marked an inline comment as done. gribozavr added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408 -public: - enum Kind { BasicBRKind, PathSensitiveBRKind }; - NoQ wrote: > gribozavr wrote: > > NoQ wro

[PATCH] D66473: Removed some dead code in BugReporter and related files

gribozavr marked 3 inline comments as done. gribozavr added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:408 -public: - enum Kind { BasicBRKind, PathSensitiveBRKind }; - NoQ wrote: > Hey, i just added that! :D (

[PATCH] D66473: Removed some dead code in BugReporter and related files

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. gribozavr added a reviewer: NoQ. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66473 Files: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h clang/includ

[PATCH] D65182: [analyzer] Add fix-it hint support.

gribozavr added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:347 + + virtual llvm::ArrayRef getFixits() const { return Fixits; } + Why is it virtual? In fact, why is BugReporter subclassable at all? ==

[PATCH] D66462: Removed the 'id' AST matcher, which is superseded by '.bind()'

This revision was automatically updated to reflect the committed changes. Closed by commit rL369380: Removed the 'id' AST matcher, which is superseded by '.bind()' (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commi

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; mgehre wrote: > gribozavr wrote: > > mgehre wrote: > > >

[PATCH] D66462: Removed the 'id' AST matcher, which is superseded by '.bind()'

gribozavr updated this revision to Diff 216082. gribozavr added a comment. Fixed a typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66462/new/ https://reviews.llvm.org/D66462 Files: clang/include/clang/ASTMatchers/ASTMatchers.h clang/unitte

[PATCH] D66462: Removed the 'id' AST matcher, which is superseded by '.bind()'

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The 'id' matcher is not even included in the AST Matchers Reference document, so I don't expect there to be a significant number of users. There's no reason to provide two ways to do the exact

[PATCH] D66270: [clang-tidy] Migrate objc-super-self to use isDerivedFrom 🚛

gribozavr added a comment. LGTM, thanks! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66270/new/ https://reviews.llvm.org/D66270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[PATCH] D66269: [clang-tidy] Migrate objc-forbidden-subclassing to use isDerivedFrom 🚛

gribozavr added a comment. LGTM, thanks! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66269/new/ https://reviews.llvm.org/D66269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[PATCH] D66350: Rudimentary support for Doxygen \retval command

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66350/new/ https://reviews.llvm.org/D66350 ___ cfe-c

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; mgehre wrote: > gribozavr wrote: > > Should this code no

[PATCH] D66212: Removed ToolExecutor::isSingleProcess, it is not used by anything

This revision was automatically updated to reflect the committed changes. Closed by commit rL368832: Removed ToolExecutor::isSingleProcess, it is not used by anything (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to co

[PATCH] D66212: Removed ToolExecutor::isSingleProcess, it is not used by anything

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. gribozavr added a reviewer: sammccall. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66212 Files: clang/include/clang/Tooling/AllTUsExecution.h clang/include/clang/Toolin

[PATCH] D66209: Improved the doc comment for getCommentsInFile

This revision was automatically updated to reflect the committed changes. Closed by commit rL368827: Improved the doc comment for getCommentsInFile (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://rev

[PATCH] D66209: Improved the doc comment for getCommentsInFile

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. gribozavr added a reviewer: hokein. hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. thanks! Repository: rG LLVM Github Monorepo https

[PATCH] D66152: Fix false negatives of statement local lifetime analysis for some STL implementation

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaInit.cpp:6579 + if (Name.size() >= 2 && Name.front() == '_' && + (Name[1] == '_' || llvm::toUpper(Name[1]) == Name[1]))

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:102 + dyn_cast_or_null(Record->getDescribedTemplate())) { +if (auto *Def = Record->getDefinition()) + addGslOwnerPointerAttributeIfNotExisting(Context, Def); I wonder why

[PATCH] D66156: Removed dead code from clang/tools/libclang/CXIndexDataConsumer.{cpp,h}

This revision was automatically updated to reflect the committed changes. Closed by commit rL368805: Removed dead code from clang/tools/libclang/CXIndexDataConsumer.{cpp,h} (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prio

[PATCH] D61027: Fix crash on switch conditions of non-integer types in templates

gribozavr added a comment. Like @riccibruno said, `check-clang-tools` will run them. However, before committing a patch, please run `check-all` -- you never know what your patch can affect. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61027/new/ https://reviews

[PATCH] D61027: Fix crash on switch conditions of non-integer types in templates

gribozavr added a comment. Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61027/new/ https://reviews.llvm.org/D61027 _

[PATCH] D66156: Removed dead code from clang/tools/libclang/CXIndexDataConsumer.{cpp,h}

gribozavr created this revision. gribozavr added a reviewer: jkorous. Herald added subscribers: cfe-commits, arphaman, dexonsmith. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66156 Files: clang/tools/libclang/CXIndexDataConsumer.cpp clang/to

[PATCH] D66136: Remove no-op callbacks from TemplateInstantiationCallback

gribozavr created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It is not clear to me that these callbacks ever did anything, they were empty even in the original implementation (r324808). Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:127 +if (!hasValidKind(Cases[I].Matcher)) + return std::vector(); +Buckets[Cases[I].Matcher.getSupportedKind()].emplace_back(I, Cases[I]); ---

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

gribozavr added a comment. In D65877#1625493 , @ymandel wrote: > I was going to add a test for `Type`/`QualType` and realized that they don't > carry any source location info. Therefore, I don't think they belong as > top-level matchers for rewrite rule

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Nice simplification, thanks! Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:127 + // in `taggedMatchers`. + std::map, 1>> + Buckets;

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaInit.cpp:6581 +if (!Callee->getIdentifier()) { + auto OO = Callee->getOverloadedOperator(); + return OO == OverloadedOp

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

gribozavr added inline comments. Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:85 // the implicit relationship of Type and QualType. static bool isBaseOf(ASTNodeKind A, ASTNodeKind B) { static auto TypeKind = ASTNodeKind::getFromNodeKind(); I'd f

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

This revision was automatically updated to reflect the committed changes. Closed by commit rL368399: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D65963: [clang][NFC] Move matcher ignoringElidableConstructorCall's tests to appropriate file.

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65963/new/ https://reviews.llvm.org/D65963 __

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

gribozavr added a comment. In D65853#1619801 , @plotfi wrote: > I tested this out. Seems to work fine, and print the Expected's Error. I am > fine with it if @compnerd and @lhames and @jkorous are cool with it. @compnerd @lhames @jkorous ping? Reposit

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

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820 + llvm::DenseMap> + CompatibleAliases; `unordered_set`? Repository: rG LLVM Gith

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6556 /// -/// ``varDecl(hasInitializer(any( -/// ignoringElidableConstructorCall(callExpr()), -/// exprWithCleanups(ignoringElidableConstructo

[PATCH] D65944: [clang] Update `ignoringElidableConstructorCall` matcher to ignore `ExprWithCleanups`.

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6538 /// -/// In C++17 copy elidable constructors are no longer being -/// generated in the AST as it is

[PATCH] D65940: [clang-format] fix crash involving invalid preprocessor line

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: unittests/Format/FormatTest.cpp:3090 Style); + // Don't crash if there is nothing following #elif. + verifyFormat("#if 1\n" --

[PATCH] D65853: Use ASSERT_THAT_ERROR instead of logAllUnhandledErrors/exit

gribozavr created this revision. gribozavr added reviewers: plotfi, jkorous, compnerd. Herald added subscribers: cfe-commits, dexonsmith, mgorny. Herald added a project: clang. ASSERT_THAT_ERROR looks like the intended helper for use in tests. Repository: rG LLVM Github Monorepo https://revie

[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323 + /*waitForInitialSync=*/false); + // llvm::Expected will throw an error if DW is an Error. + if (!DW) plotfi wrote: > gribozavr wrote: > > plotf

[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323 + /*waitForInitialSync=*/false); + // llvm::Expected will throw an error if DW is an Error. + if (!DW) plotfi wrote: > gribozavr wrote: > > plotf

[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323 + /*waitForInitialSync=*/false); + // llvm::Expected will throw an error if DW is an Error. + if (!DW) plotfi wrote: > gribozavr wrote: > > Call

[PATCH] D65829: [clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:323 + /*waitForInitialSync=*/false); + // llvm::Expected will throw an error if DW is an Error. + if (!DW) Call `llvm::cantFail` and there will be no

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287 /*waitForInitialSync=*/true); + if (!DW) return; plotfi wrote: > gribozavr wrote: > > plotfi wrote: > > > gribozavr wrote: > > > > Why? This i

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

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:7077 +// someContainer.add(std::move(localOWner)); +// return p; +if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) && xazax.hun wrote: > gribozavr wrote: > > Why is

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case it

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. `-Weverything` is not recommended for anything except compiler testing, and similarly with clang-tidy, enabling all checkers is not a good idea. I don't think improving this particular point will make enabling all checks more usable. Repository: rCTE Clang Tools Ex

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. > This suggestion would result another strange behavior: if the user disables > cert-err09-cpp because he or she doesn't want to see its reports, the other > one (cert-err61-cpp) will still report the issue. So he or she has to disable > both (or as many aliases it ha

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. Producing the message two times is worse user experience than producing one. Most users don't care which checker produced the message. However, the output should be deterministic. Therefore, a better fix would be making deduplication deterministic, instead of printing

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. So the problem you're trying to solve is that the output is non-deterministic, is that correct? > However, it is random which checker name is included in the warning. If that is indeed the problem, then I think the solution should be making the output deterministic -

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6581 +if (!Callee->getIdentifier()) { + auto OO = Callee->getOverloadedOperator(); + return OO == OverloadedOperatorKind::OO_Subscript || xazax.hun wrote: > gribozavr wrote: >

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283 /*waitForInitialSync=*/true); + if (!DW) return; plotfi wrote: > jkorous wrote: > > jkorous wrote: > > > IIUC this is silently dropping errors. We

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

gribozavr added inline comments. Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102 - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case it

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

gribozavr added inline comments. Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:12 +the loop condition is not changed. This check detects such loops. A loop is +considered as infinite if it does not have any loop exit statement (``break``, +``continue``, ``goto``,

[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6572 +return false; + return llvm::StringSwitch(Callee->getName()) + .Cases("begin", "rbegin", "cbegin", "crbegin", true) xazax.hun wrote: > gribozavr wrote: > > Should we also che

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

gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:7031 + LLVM_FALLTHROUGH; +case IndirectLocalPathEntry::DefaultInit: return Path[I].E->getSourceRange(); This change would be best committed

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

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Sema/Sema.h:6122 + /// std::container::iterator. \param UnderlyingRecord The record named by ND. + void addDefaultGslPointerAttrib

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6581 +if (!Callee->getIdentifier()) { + auto OO = Callee->getOverloadedOperator(); + return OO == OverloadedOperatorKind::OO_Subscript || mgehre wrote: > xazax.hun wrote: > > I

[PATCH] D64907: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/AST/RecursiveASTVisitorTest.cpp:107 +} \ No newline at end of file Please add a newline. Repository: rG LLVM Githu

[PATCH] D64696: Adds a warning when an inline Doxygen comment has no argument

This revision was automatically updated to reflect the committed changes. Closed by commit rL367809: Adds a warning when an inline Doxygen comment has no argument (authored by gribozavr, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit

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

gribozavr added inline comments. Comment at: clang/include/clang/Sema/Sema.h:6097 + + /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types. + void addDefaultGslPointerAttribute(TypedefNameDecl *TD); It seems like this function does not add gsl

[PATCH] D63954: Add lifetime categories attributes

gribozavr added inline comments. Comment at: cfe/trunk/include/clang/Basic/AttrDocs.td:4252 + +The argument ``T`` is optional and currently ignored. +This attribute may be used by analysis tools and has no effect on code "and is currently ignored" even better:

[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6563 +static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { + if (auto *Conv = dyn_cast_or_null(Callee)) This looks like an ad-hoc rule. Is there a way to express it wit

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

gribozavr accepted this revision. gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:7095 auto *MTE = dyn_cast(L); + if (IsGslPtrInitWithGslTempOwner) { +Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange; I

[PATCH] D64696: Adds a warning when an inline Doxygen comment has no argument

gribozavr accepted this revision. gribozavr added a comment. This revision is now accepted and ready to land. Thank you! Do you have commit access or would you like me to commit this patch for you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64696/new/ https://reviews.llvm.org/D64696

[PATCH] D64907: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

gribozavr added a comment. Looks like a good idea to me. Regarding tests, I couldn't find existing tests that check order either. Seems like you'd need to make some minimal infrastructure for that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D649

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

gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332 S->isSemanticForm() ? S->getSyntacticForm() : S, Queue)); TRY_TO(TraverseSynOrSemInitListEx

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

gribozavr added inline comments. Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:25 + +static bool isAccessForVar(const Stmt *St, const VarDecl *Var) { + if (const auto *DRE = dyn_cast(St)) St => S "S" is a common abbreviation in the Clang codebase, "St"

[PATCH] D64696: Adds a warning when an inline Doxygen comment has no argument

gribozavr added a comment. Is it true that all inline commands require an argument? For example, "\&" does not. Comment at: clang/test/Sema/warn-documentation.cpp:1030 +// The inline comments expect a string after the command. +// expected-warning@+1 {{'\a' command does not

[PATCH] D63954: Add lifetime categories attributes

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 wro

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

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)); TRY_TO(TraverseSynOrSemInitListExpr

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

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 looke

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

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: > griboz

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

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 (partial

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

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 relate

[PATCH] D63954: Add lifetime categories attributes

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 b

<    4   5   6   7   8   9   10   11   12   >