[PATCH] D88172: [clangd] Extract common file-caching logic from ConfigProvider.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp:15 +#include +#include + kbobyrev wrote: > Please sort the includes here (clangd should have a Code Action here). The includes are sorted, this is a bug in

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 updated this revision to Diff 307549. njames93 added a comment. Tweak disableUnusableChecks to take an ArrayRef over a vector. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-e

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 updated this revision to Diff 307557. njames93 added a comment. . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdS

[PATCH] D88172: [clangd] Extract common file-caching logic from ConfigProvider.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/support/FileCache.cpp:59 + // If the modified-time and size match, assume the content does too. + if (Size == Stat->getSize() && + ModifiedTime == Stat->getLastModificationTime()) Is trac

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 updated this revision to Diff 307565. njames93 added a comment. Use DefaultOptionsProvider for initializing ClangTidyContext. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-ex

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 marked 4 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.h:25 + /*Priority=*/unsigned &, + /*CWD*/ PathRef); +} // namespace detail

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 updated this revision to Diff 307615. njames93 added a comment. Removed duplicated logging of options. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-extra/clangd/CMakeLists.t

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.h:25 +/*Filename=*/llvm::StringRef, +/*CWD*/ PathRef) const>; + sammccall wrote: > CW

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.h:25 +/*Filename=*/llvm::StringRef, +/*CWD*/ PathRef) const>; + sammccall wrote: > sa

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 updated this revision to Diff 307642. njames93 added a comment. Removed CWD references. Added some fixme comments about function_ref<->unique_function and null values. Added asserts in provideClangTidyFiles ensuring path is absolute. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D91029: [clangd] Implement clang-tidy options from config

This revision was automatically updated to reflect the committed changes. Closed by commit rG73fdd998701c: [clangd] Implement clang-tidy options from config (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://rev

[PATCH] D91029: [clangd] Implement clang-tidy options from config

njames93 added a comment. Whoops, probably should've updated the summary before pushing, ah well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 ___ cfe-commits

[PATCH] D89065: [clang] Tweaked fixit for static assert with no message

njames93 added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89065/new/ https://reviews.llvm.org/D89065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34 + llvm::Optional + get(const ThreadsafeFS &TFS, + std::chrono::steady_clock::time_point FreshTime) const { To save a copy, could this not return a const pointer to wha

[PATCH] D92155: Load plugins when creating a CompilerInvocation.

njames93 added a comment. Is it possible to write tests for this, obviously they will only work with plugin support on the current platform. Also now that this is loaded from the command line, how will this play nicely with other tools that haven't been built with plugin support? Repository:

[PATCH] D92157: [clangd] Add language metrics for recovery AST usage.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/Selection.cpp:40 +std::string getLanguage(const clang::LangOptions &Lang) { + if (Lang.C99 || Lang.C11 || Lang.C17 || Lang.C2x) Nit: any reason to use std::string here instead StringRef. R

[PATCH] D92175: [clang-tidy] Add support for loading configs for specific files.

njames93 created this revision. njames93 added reviewers: aaron.ballman, alexfh, gribozavr2. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, xazax.hun. Herald added a project: clang. njames93 requested review of this revision. There was already a basic version of this in Iden

[PATCH] D92175: [clang-tidy] Add support for loading configs for specific files.

njames93 added a comment. In D92175#2418651 , @sammccall wrote: > Currently AFAICS there's just *one* check that ever reads the config for > other files, and it could be redesigned not to do so - by conservatively > renaming only identifiers declared in

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34 + llvm::Optional + get(const ThreadsafeFS &TFS, + std::chrono::steady_clock::time_point FreshTime) const { sammccall wrote: > njames93 wrote: > > To save a copy, could

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

njames93 added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113 +OptionStack.push_back(std::move(*Config)); +if (!OptionStack.back().InheritParentConfig) + break; njames93 wrote: > sammccall wrote: > > njames93 w

[PATCH] D92133: [clangd] Cache .clang-tidy files again.

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. LGTM, with a couple of nits that could easily be disregarded. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-105 +for (auto I = path::begin(Parent, path::St

[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

njames93 updated this revision to Diff 308121. njames93 added a comment. I thought I'd bring this back up. - Rebased and tweaked to use a few new library features in clang-tidy since when this was first proposed. - It definitely has value but still maybe got a little bit before its ready to lan

[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

njames93 updated this revision to Diff 308122. njames93 added a comment. Fix failing test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73413/new/ https://reviews.llvm.org/D73413 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt cl

[PATCH] D84924: [clang-tidy][WIP] Added command line option `fix-notes`

njames93 updated this revision to Diff 308153. njames93 added a comment. Fix documentation. Ping?? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84924/new/ https://reviews.llvm.org/D84924 Files: clang-tools-extra/clang-tidy/ClangTidy.cpp clan

[PATCH] D92267: [clang-tidy][NFC] Use moves instead of copies when constructing OptionsProviders.

njames93 created this revision. njames93 added reviewers: aaron.ballman, alexfh. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. njames93 requested review of this revision. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D92267 Files: clang-tools

[PATCH] D92268: [clang-tidy][NFC] Skip intermediate cache directories when inheriting configs.

njames93 created this revision. njames93 added reviewers: alexfh, aaron.ballman, gribozavr2. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. njames93 requested review of this revision. If an configuration specifies InheritParentOption and is cached in child direct

[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

njames93 updated this revision to Diff 308165. njames93 added a comment. Removed warnings on template definitions. Added some basic fix-it support, need some lexer magic to create the fix-it for declarations declared with extern. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92267: [clang-tidy][NFC] Use moves instead of copies when constructing OptionsProviders.

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:176 public: - DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions, - const ClangTidyOptions &Options) - : GlobalOptions(GlobalOptions), Defaul

[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

njames93 updated this revision to Diff 308176. njames93 added a comment. Added fixit to remove extern. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73413/new/ https://reviews.llvm.org/D73413 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.t

[PATCH] D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic

njames93 added a comment. This small fix is definitely worth while, the more involved patch may not ever land. Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:322 DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage) -

[PATCH] D92272: [clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92272/new/ https://reviews.llvm.org/D92272 _

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

njames93 added a comment. Can this be moved from bugprone to the new concurrency module added in D91656 . Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75229/new/ https://reviews.llvm.org/D75229 ___

[PATCH] D92041: [clangd] Add hover info for `this` expr

njames93 added a comment. One last point, is it worth including cv qualifications in the hover info? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92041/new/ https://reviews.llvm.org/D92041 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

njames93 updated this revision to Diff 309093. njames93 added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix assertion due to a pointer stored in a ManagedStatic. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D9

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

njames93 added reviewers: chandlerc, dblaikie. njames93 added inline comments. Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:190-196 +// Use a custom deleter for the TrueMatcherInstance ManagedStatic. This prevents +// an assert firing when the refcount is nonzero wh

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

njames93 updated this revision to Diff 309120. njames93 added a comment. Added test cases demonstrating copy and move behaviour. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92480/new/ https://reviews.llvm.org/D92480 Files: clang/lib/ASTMatcher

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

njames93 added a comment. I've added test cases which demonstrate what these copies/moves are about. The move constructor probably would never get used. I can't see of a reason to move the contents of a ref counted pointer. In which case there could be an argument to delete it. The copy construc

[PATCH] D91925: [clangd][NFC] Small tweak to combined provider

njames93 updated this revision to Diff 309486. njames93 added a comment. Removed const qualifier on Providers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91925/new/ https://reviews.llvm.org/D91925 Files: clang-tools-extra/clangd/ConfigProvide

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

njames93 updated this revision to Diff 309489. njames93 added a comment. Made this just focus on the destruction asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92480/new/ https://reviews.llvm.org/D92480 Files: clang/lib/ASTMatchers/ASTMa

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 added inline comments. Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:190-196 +// Use a custom deleter for the TrueMatcherInstance ManagedStatic. This prevents +// an assert firing when the refcount is nonzero while running its destructor. +struct DynMatcherI

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 updated this revision to Diff 309512. njames93 added a comment. Made ManagedStatic code transparent to users for RefCounted objects. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92480/new/ https://reviews.llvm.org/D92480 Files: clang/l

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 created this revision. njames93 added reviewers: Eugene.Zelenko, aaron.ballman. Herald added subscribers: kbarton, xazax.hun, nemanjai. njames93 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Using bools instead of integers better

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 updated this revision to Diff 309519. njames93 added a comment. Whoops missed one Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92652/new/ https://reviews.llvm.org/D92652 Files: clang-tools-extra/docs/clang-tidy/checks/bugprone-argument

[PATCH] D91885: [clang-tidy] Add support for diagnostics with no location

njames93 marked 2 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:276 + // Never ignore these. +} else if (!Context.isCheckEnabled(Error.DiagnosticName) && + Error.DiagLevel !

[PATCH] D91885: [clang-tidy] Add support for diagnostics with no location

njames93 updated this revision to Diff 309521. njames93 added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91885/new/ https://reviews.llvm.org/D91885 Files: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp clang-to

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 updated this revision to Diff 309524. njames93 added a comment. Remove unnecessary comment added in ASTMatchersInternal.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92480/new/ https://reviews.llvm.org/D92480 Files: clang/lib/ASTMat

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 marked 3 inline comments as done. njames93 added a subscriber: Charusso. njames93 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:60 - If copy to the destination array can overflow [1] and - ``AreSafe

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 updated this revision to Diff 309605. njames93 added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92652/new/ https://reviews.llvm.org/D92652 Files: clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 marked 6 inline comments as done and 2 inline comments as done. njames93 added inline comments. Comment at: llvm/include/llvm/Support/ManagedStatic.h:25 +// that are const with no params. +template struct HasRetainRelease { +private: dblaikie wrote: > A

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 removed a subscriber: Charusso. njames93 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:60 - If copy to the destination array can overflow [1] and - ``AreSafeFunctionsAvailable`` is set to ``Yes``, `

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

njames93 updated this revision to Diff 309661. njames93 added a comment. Update bugprone-not-null-terminated-result incorrect option name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92652/new/ https://reviews.llvm.org/D92652 Files: clang-tools

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:235-239 + for (const auto &Opt : Options) { +std::string Val = HNOption.General.lookup(Opt.first); +if (Val.empty()) + HNOption.General.insert({Opt.first,

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 added inline comments. Comment at: llvm/include/llvm/Support/ManagedStatic.h:25 +// that are const with no params. +template struct HasRetainRelease { +private: dblaikie wrote: > njames93 wrote: > > dblaikie wrote: > > > Are there many uses that rely on

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

njames93 updated this revision to Diff 309730. njames93 added a comment. Refactored the ManagedStatic instance into a function static. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92480/new/ https://reviews.llvm.org/D92480 Files: clang/lib/ASTM

[PATCH] D92749: [clangd] go-to-implementation on a base class jumps to all subclasses.

njames93 added a comment. How does this handle Dependent base classes (including CRTP). Can tests be added to demonstrate that behaviour if its supported. Comment at: clang-tools-extra/clangd/XRefs.cpp:1200 + } +if (const auto *D = dyn_cast(ND)) { + IDs.insert(ge

[PATCH] D92663: [clangd] Add hot-reload of compile_commands.json and compile_flags.txt

njames93 added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:712 + // - .clang_format and .clang-tidy + // - compile_commands.json } Any reason for re-specifying compile_commands.json here? Comment at: clang-too

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

njames93 added a comment. This needs rebasing against main. Can't be applied cleanly in its current state. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:398-404 + std::string OptionVal = StrMap.lookup(OptionKey); + llvm::transform(OptionVal,

[PATCH] D92753: [clang-format] Add IndentPragma style to eliminate common clang-format off scenario

njames93 added a comment. Should this be controlled on a case by case basis, maybe control indentation using a regex over the pragma arguments, WDYT? Comment at: clang/docs/ClangFormatStyleOptions.rst:1923-1924 + + When ``false``, pragmas are flushed left or follow IndentPPDi

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

This revision was automatically updated to reflect the committed changes. Closed by commit rG980618145bf0: [clang-tidy][docs] Update check options with boolean values instead of non… (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D92756: [clang-tidy] Support all YAML supported spellings for bools in CheckOptions.

njames93 created this revision. njames93 added a reviewer: aaron.ballman. Herald added a subscriber: xazax.hun. njames93 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Depends on D92755 Repository: rG

[PATCH] D92756: [clang-tidy] Support all YAML supported spellings for bools in CheckOptions.

njames93 updated this revision to Diff 309985. njames93 added a comment. Refactored based on changes to parent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92756/new/ https://reviews.llvm.org/D92756 Files: clang-tools-extra/clang-tidy/ClangTid

[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

This revision was automatically updated to reflect the committed changes. Closed by commit rGdc361d5c2a2d: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D92480?vs=309730&id=309989#toc Repository: rG LLV

[PATCH] D92788: [clangd] NFC: Use SmallVector where possible

njames93 added a comment. Not sure I'm a huge fan of this, Some of these cases the size is specified because that's the upper limit we typically expect the SmallVector to use. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92788/new/ https://review

[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

njames93 added a comment. How does this type alias and typedef, In theory that should also be handled. using Functor = std::function<...>; Functor Copy = Orig; // No warning. Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:39-44 +AST

[PATCH] D87830: [clang-tidy][test] Allow empty checks in check_clang_tidy.py

njames93 added a comment. Probably not quite as verbose but should do the job // RUN: clang-tidy %s --checks=-*,my-check-to-test --warnings-as-errors=* -- -std=c++11 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87830/new/ https://reviews.llvm.

[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp:409 + +namespace std { + gribozavr2 wrote: > flx wrote: > > gribozavr2 wrote: > > > Could you add a nested inline namespace to

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

njames93 added a comment. It may be a pain, but I'd also like to see an option like `RemovePrefix`. Generally most unscoped enums constants have prefixes based on the enum name. A simple way around this would just be to get the longest common prefix in all the enums constants. There is probably

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums-over-unscoped

njames93 added a comment. In D85697#2234468 , @janosbenjaminantal wrote: > The known "limitations" are mostly similar to other checks: > > - It cannot detect unfixable cases from other translation units. Practically > that means if an `enum` is used in m

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

njames93 added a comment. Should point out there is already a check for cert-oop57-cpp, added in D72488 . Do these play nice with each other or should they perhaps be merged or share code? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

njames93 added a comment. Come to think of it, this is a pretty illogical way to solve this problem, just append `::std::function` to the AllowedTypes vector in `registerMatchers` and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simp

[PATCH] D89065: [clang] Tweaked fixit for static assert with no message

njames93 added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89065/new/ https://reviews.llvm.org/D89065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D79674: [clang-tidy] Better support for Override function in RenamerClangTidy based checks

This revision was automatically updated to reflect the committed changes. Closed by commit rG866dc0978449: [clang-tidy] Better support for Override function in RenamerClangTidy based… (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D79674?vs=298616&id=299043#toc Repos

[PATCH] D89407: [clang-tidy] Add scoped enum constants to identifier naming check

This revision was automatically updated to reflect the committed changes. Closed by commit rG86ef37980016: [clang-tidy] Add scoped enum constants to identifier naming check (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D89407?vs=298349&id=299058#toc Repository: rG

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums

njames93 added a comment. In D85697#2339055 , @aaron.ballman wrote: > I tend to be very skeptical of the value of checks that basically throw out > entire usable chunks of the base language because the check is effectively > impossible to apply to an ex

[PATCH] D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums

njames93 added a comment. In D85697#2340317 , @janosbenjaminantal wrote: > In D85697#2338249 , @njames93 wrote: > >> In D85697#2234468 , >> @janosbenjaminantal wrote: >> >>

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

njames93 added a comment. In D86671#2341838 , @dougpuob wrote: > Hi @njames93, > > It's a smart idea, the rework for it is worth. There is a special case if > lowercase name with Hungarian prefix, it possibly makes variable ambiguous, > like the `Case1`.

[PATCH] D72553: [clang-tidy] Add performance-prefer-preincrement check

njames93 updated this revision to Diff 299443. njames93 added a comment. Rebased and fix issues relating to rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72553/new/ https://reviews.llvm.org/D72553 Files: clang-tools-extra/clang-tidy/llvm/

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

njames93 added inline comments. Comment at: clang-tools-extra/clangd/FindSymbols.cpp:182 index::SymbolInfo SymInfo = index::getSymbolInfo(&ND); - // FIXME: this is not classifying constructors, destructors and operators - //correctly (they're all "methods"). + // FI

[PATCH] D89852: [clangd] Use SmallString instead of std::string in marshalling code; NFC

njames93 added a comment. I'm a little concerned by this, not sure if `Expected` and `Optional` play nicely with rvo. If the compiler can't optimise it's potentially a 256 byte memcpy in the return, granted that would still be cheaper than dynamic allocation. However it would most likely be fas

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

njames93 added a comment. Isn't `llvm::errs()` buffered, negating most of the benefit here. Also using std::string here is bad, its potentially going to allocate and reallocate memory each time it grows. It would be better off using an `llvm::SmallString` and looking at what could potentially be

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

njames93 created this revision. njames93 added a reviewer: gribozavr2. Herald added a project: clang. Herald added a subscriber: cfe-commits. njames93 requested review of this revision. Rearrange the fields to reduce the size of the classes Repository: rG LLVM Github Monorepo https://reviews.

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

njames93 updated this revision to Diff 300622. njames93 added a comment. Keep VerbatimBlockEndCommandName after LexerState while preserving smaller size Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90127/new/ https://reviews.llvm.org/D90127 Files

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

njames93 added inline comments. Comment at: clang/include/clang/AST/CommentLexer.h:244 + /// command, including command marker. + SmallString<16> VerbatimBlockEndCommandName; + gribozavr2 wrote: > I'm not a fan of this change to `Lexer` because it breaks the gr

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

njames93 updated this revision to Diff 300623. njames93 added a comment. Fix field initialisation order in constructor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90127/new/ https://reviews.llvm.org/D90127 Files: clang/include/clang/AST/Commen

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

njames93 added a comment. In D80531#2352944 , @kkleine wrote: > @dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will > "Stop and fail the build, if a compiler warning is triggered. Defaults to > OFF." I wonder if any other test fails

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

njames93 added a comment. In D80531#2353314 , @kkleine wrote: > @njames93 I could do that but the original Macros had were defined without a > semicolon at the end and one had to add it manually. See this revision in > which I replaced some occurrences o

[PATCH] D90180: [clang-tidy] find/fix unneeded semicolon after switch

njames93 added a comment. Is this not already handled by `-Wextra-semi`. If it isn't I feel like it should be handled there rather than in clang-tidy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90180/new/ https://reviews.llvm.org/D90180 ___

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

This revision was automatically updated to reflect the committed changes. Closed by commit rGb698ad00cbc7: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D90127?vs=300623&id=300831#toc Repository

[PATCH] D90180: [clang-tidy] find/fix unneeded semicolon after switch

njames93 added a comment. Taking a step back, clang-tidy checks are supposed to enforce guidelines for the specific module they live in. If there are 10'000 occurrences of a semi directly after a switch closing brace in the linux kernel code base it could be argued that its a style guideline of

[PATCH] D90244: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations

njames93 created this revision. njames93 added a reviewer: aaron.ballman. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. njames93 requested review of this revision. Remove the need to heap allocate a string for each style option lookup while reading or writing op

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

njames93 added a comment. If you plan on contributing quite a lot then it would be wise to upload your patches with arcanist - https://llvm.org/docs/Phabricator.html. It will help prevent issues with diffs being relative to previous revisions. Personally I just create a branch from master for a

[PATCH] D90244: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations

njames93 added a comment. In D90244#2357130 , @aaron.ballman wrote: > Was this caused by a performance concern when profiling something? I'm not > opposed to the changes, but I do think the original formulation is easier to > read. It's not a huge perf

[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

njames93 added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6763 +/// (but not "nodiscard" or "clang::warn_unused_result"). +AST_MATCHER_P(Attr, hasAttrName, llvm::StringRef, Name) { + return Node.getAttrName() && Node.getAttrName()->isStr(Name); --

[PATCH] D90303: [ASTMatchers] Made isExpandedFromMacro Polymorphic

njames93 created this revision. njames93 added reviewers: klimek, aaron.ballman. Herald added a project: clang. Herald added a subscriber: cfe-commits. njames93 requested review of this revision. Made the isExpandedFromMacro matcher work on Stmt's, TypeLocs and Decls in line with the other macro

[PATCH] D90244: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations

This revision was automatically updated to reflect the committed changes. Closed by commit rG556ee675c147: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D90244?vs=301032&id=301300#toc Reposito

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

njames93 added a comment. It's also be nice if --config-file would also support being passed a directory. If a directory was passed it would append ".clang-tidy" to the path and load that file, WDYT? Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:320-324 +

[PATCH] D90392: [clang-tidy] Omit std::make_unique/make_shared for default initialization.

njames93 requested changes to this revision. njames93 added a comment. This revision now requires changes to proceed. IIUC, this is handling the case where `Ptr.reset(new int)` which is different to `Ptr.reset(new int())` because the former doesn't initialise the int while the latter default(zer

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:329-330 + + if (!ConfigFile.empty()) { +if (!Config.empty()) { + llvm::errs() << "Error: --config-file and --config are " nit: Should we be using `Config(Fil

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

njames93 added a comment. Can you rebase this against trunk please, having trouble testing it out locally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86671/new/ https://reviews.llvm.org/D86671 ___ cfe

[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:441-456 +static IdentifierNamingCheck::HungarianPrefixOption +parseHungarianPrefix(std::string OptionVal) { + for (auto &C : OptionVal) +C = toupper(C); + + if (s

<    5   6   7   8   9   10   11   12   13   14   >