[PATCH] D147876: [clang-tidy] Support introducing checks as a list in the config file

2023-04-09 Thread Nathan James via Phabricator via cfe-commits
njames93 requested changes to this revision. njames93 added a comment. This revision now requires changes to proceed. From an architectural point of view, is there any reason we don't change the backend to treat checks internally as a vector? `clang-tools-extra/docs/clang-tidy/index.rst` also ne

[PATCH] D147924: [clang-tidy] Exclude template instantiations in modernize-use-override

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I'm not sure this is really a great improvement. If the override candidate originates in a templated base class I think we should still warn struct Foo { virtual void foo(); }; template struct DirectBase : T { virtual void foo(); }; DirectBase

[PATCH] D147876: [clang-tidy] Support specifying checks as a list in the config file

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:129-141 +// Special case for reading from YAML +// Must support reading from both a string or a

[PATCH] D147918: [clang-tidy] Added IgnoreVirtual option to misc-unused-parameters

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Have you thought about handling CRTP overrides template class Base { int getThing(int x) { return x; } }; class Derived : public Base { int getThing(int x) { return 0; } }; I'm not saying update this to work for that, but could

[PATCH] D141892: Implement modernize-use-constraints

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a reviewer: carlosgalvezp. njames93 added a comment. Would you consider supporting enable_if via parameters template void doStuff(T&, std::enable_if_t = nullptr) {} Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:71 + +return s

[PATCH] D147924: [clang-tidy] Exclude template instantiations in modernize-use-override

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147924#4255209 , @PiotrZSL wrote: > Problem is that you may have specialization of Bar class wihtout bar base, > and that specialization can be local to some compilation unit. > Like: > > template<> > struct Bar { >

[PATCH] D147918: [clang-tidy] Added IgnoreVirtual option to misc-unused-parameters

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
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/D147918/new/ https://reviews.llvm.org/D147918 __

[PATCH] D144347: [clang-tidy] Add readability-forward-usage check

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 requested changes to this revision. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ForwardUsageCheck.cpp:45 + +inline SourceRange getAnglesLoc(const Expr *MatchedCallee) { + if (const auto *DeclRefExprCallee = Use `

[PATCH] D107294: [clang-tidy] adds warning to suggest users replace symbols with words

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. The for Pipe/BitwiseOr issue. One heuristic could be: - If it has any kind of template dependence don't diagnost - If its a `BinaryOperator`, its safe to diagnose. - If its a `CxxOperatorCallExpr`, the simplest case I can think of is to check the spelling of the operato

[PATCH] D147955: [clang-tidy] Extend CheckOptions to support grouping checks options

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: carlosgalvezp, LegalizeAdulthood, aaron.ballman. Herald added subscribers: arphaman, mgrang, xazax.hun. Herald added a project: All. njames93 requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a p

[PATCH] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Have you thought of the possibility of handling this case takeOptionalValue(std::move(*param)); Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst:34 +Value extraction using ``operator *`` is matched by defa

[PATCH] D147876: [clang-tidy] Support specifying checks as a list in the config file

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147876#4256155 , @carlosgalvezp wrote: > Thanks for the review @njames93 ! Do you think it makes sense that we > deprecate the string format, so that we only support the list format? To be > fully removed in clang-tidy 19.

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 512267. njames93 added a comment. Added dynamic checking for existence of isa_and_present before suggesting it as a replacement Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131319/new/ https://reviews.llvm.o

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 512269. njames93 added a comment. Herald added a subscriber: ChuanqiXu. Fix wrong patch upload Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131319/new/ https://reviews.llvm.org/D131319 Files: clang-tools-e

[PATCH] D147955: [clang-tidy] Extend CheckOptions to support grouping checks options

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147955#4256649 , @PiotrZSL wrote: > There are so many ways of defining check configurations, and I never heard of > them. > Maybe some separate section in documentation is needed for them. The current documentation only def

[PATCH] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147357#4256636 , @PiotrZSL wrote: > As for `takeOptionalValue(std::move(*param));`, it could be added as an > configuration option for PassthroughFunctions or UtilityFunctions. I can > thing about that, that should be easy

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. @PiotrZSL Thank you for your review, however we tend to only comment on code that has been changed by the patch. You have made some good points about parts of this check, but they can be addressed in an NFC commit. Comment at: clang-tools-extra/clan

[PATCH] D147876: [clang-tidy] Support specifying checks as a list in the config file

2023-04-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147876#4257197 , @carlosgalvezp wrote: > In D147876#4256182 , @njames93 > wrote: > >> with checked in configuration files > > I'm not sure I understand what you mean by "checked in"

[PATCH] D148110: [clang-tidy] Ctor arguments are sequenced if ctor call is written as list-initialization.

2023-04-13 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1163 +namespace { + Whats with this namespace addition? looks unnecessary and should be removed Comment at: clang-tools-extra

[PATCH] D147802: [clangd] Handle destructors in DefineOutline tweak

2023-04-13 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:186-194 + if (const auto *Destructor = llvm::dyn_cast(FD)) { +if (auto Err = DeclarationCleanups.add(tooling::Replacement( +SM, Destructor->getLocation(), 0, +

[PATCH] D147802: [clangd] Handle destructors in DefineOutline tweak

2023-04-14 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG2eb9cc76a6c0: [clangd] Handle destructors in DefineOutline tweak (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D147802?vs=511780&id=513644#toc Repository: rG LLVM Github M

[PATCH] D147808: [clangd] Support defaulted destructors in Define Outline tweak

2023-04-14 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D147808#4268523 , @kadircet wrote: > there's actually a slight difference between an inline defaulted special > member function and an out-of-line defaulted one. the latter makes the > special member "user-defined" which mig

[PATCH] D147808: [clangd] Support defaulted destructors in Define Outline tweak

2023-04-14 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 513669. njames93 added a comment. Updated to work on all special member functions as long as they aren't trivial Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147808/new/ https://reviews.llvm.org/D147808 File

[PATCH] D148318: [clang-tidy] Add `performance-avoid-endl` check

2023-04-14 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. For what its worth, there typically isn't any performance gain using `\n` over `std::endl` when writing to `std::cerr` or `std::clog` (or their wide string counterparts) as every write operation on those implicitly calls flush. However If the fix was changed to convert

[PATCH] D147808: [clangd] Support non-trivial defaulted special member functions in Define Outline tweak

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 513862. njames93 edited the summary of this revision. njames93 added a comment. Fix unittest, tests had trivial and non trivial mixed up Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147808/new/ https://review

[PATCH] D148423: [clangd] Support define outline tweak for non cross file edits

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: kadircet, sammccall, nridge. Herald added a subscriber: arphaman. Herald added a project: All. njames93 requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-ext

[PATCH] D148423: [clangd] Support define outline tweak for non cross file edits

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Theres definitely scope to improve this in follow ups. We could support this for template methods and classes template struct A { void [[Foo]]() {} template void [[Bar]]() {} }; // Both ranges transformed to template struct A { void Foo(); te

[PATCH] D144828: [clang-tidy] Add misc-header-include-cycle check

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:134-135 +do { + if (CurrentIt->Name.empty()) +continue; + Is this a case that can ever come up? If it does come up, surely any notes emitte

[PATCH] D146842: [clang-tidy] Fix crash when handling invalid config values

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidy.h:61-69 struct NamesAndOptions { llvm::StringSet<> Names; llvm::StringSet<> Options; + std::vector Errors; }; NamesAndOptions I don't think this is the correct approach

[PATCH] D146842: [clang-tidy] Fix crash when handling invalid config values

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 requested changes to this revision. njames93 added a comment. This revision now requires changes to proceed. Ok, taking a step back here, this patch is trying to address 2 unrelated things at once: - using `--dump-config` with bad check options results in a crash - using `--verify-confi

[PATCH] D144828: [clang-tidy] Add misc-header-include-cycle check

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. LGTM: Just one small nit. Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:148-150 +return std::any_of( +IgnoredFilesRegexes.begin()

[PATCH] D141892: Implement modernize-use-constraints

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:63-65 +std::string Name = TheType.getType().getAsString(); +if (Name.find("enable_if<") == std::string::npos) + return std::nullopt; This strin

[PATCH] D148318: [clang-tidy] Add `performance-avoid-endl` check

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlEndsCheck.cpp:28 + declRefExpr( + to(namedDecl(hasAnyName("endl", "ends")).bind("decl"))) + .bind("expr", Why are you

[PATCH] D146842: [clang-tidy] Fix crash when handling invalid config values

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D146842#4271144 , @PiotrZSL wrote: > In D146842#4271089 , @njames93 > wrote: > >> I think this should be split up into 2 separate patches, one for each of the >> proposed changes and

[PATCH] D148423: [clangd] Support define outline tweak for non cross file edits

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 513911. njames93 added a comment. - Clang format changes - Update apply logic to not query the FS when its not a cross file edit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148423/new/ https://reviews.llvm.o

[PATCH] D148436: [clang-tidy] Fix crash in --dump-config

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: PiotrZSL, carlosgalvezp. Herald added a subscriber: xazax.hun. Herald added a project: All. njames93 requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Fixes a crash in dump

[PATCH] D148436: [clang-tidy] Fix crash in --dump-config

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. @PiotrZSL I hope you don't mind, I've put just the crash fix up here for D146842 and marked you as the coauthor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148436/new/ https://reviews

[PATCH] D148436: [clang-tidy] Fix crash in --dump-config

2023-04-15 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG6128bcd02512: [clang-tidy] Fix crash in --dump-config (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148436/new/ https://reviews.llvm

[PATCH] D143342: [clang-tidy] Support std::format and std::print in readability-redundant-string-cstr

2023-02-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:188 + callee(functionDecl(hasName("::std::format", +hasAnyArgument(materializeTemporaryExpr( +

[PATCH] D143342: [clang-tidy] Support std::format and std::print in readability-redundant-string-cstr

2023-02-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Just a few more points then it should be good to land Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:181-190 + + // Detect redundant 'c_str()' calls in parameters passed to std::print and + // std::format. + Finde

[PATCH] D149723: [clang-tidy] Optimize performance of RenamerClangTidyCheck

2023-05-05 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. Mostly looks good, just a few small nits Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:47 -std::hash SecondHash; -return DenseMapInfo

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

2023-05-06 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:20 +if (ReplacementPrintFunction == "std::print") + return LangOpts.CPlusPlus2b; +return LangOpts.CPlusPlus; This will need rebasing and changing to

[PATCH] D150254: [tidy] Fix possible use-after-free in IdentifierNamingCheck

2023-05-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D150254#4331640 , @kadircet wrote: > see > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L638 > for such a pattern, clangd also initializes checks with a similar approach. In this

[PATCH] D150254: [tidy] Fix possible use-after-free in IdentifierNamingCheck

2023-05-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D150254#4331734 , @hokein wrote: > The fix looks good. > > We also have a `CheckName` field in the base class `ClangTidyCheck`, however > that's field is private, we can't not access, we could consider make it > protected (I

[PATCH] D150126: [clang-tidy][WIP] Set traversal scope to prevent analysis in headers

2023-05-10 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: carlosgalvezp, PiotrZSL. Herald added a subscriber: xazax.hun. Herald added a project: All. njames93 updated this revision to Diff 520620. njames93 added a comment. njames93 updated this revision to Diff 520918. njames93 retitled this revisi

[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo

2023-05-18 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:97 : ClangTidyCheck(Name, Context), + UseV2Style(Options.getLocalOrGlobal("UseV2Style", false)), Handler(std::make_unique( Given this check is pa

[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

2023-04-26 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. It may be a good idea to create unittests for the formatconverter in `clang-tools-extra/unittests/clang-tidy/` Can this check be renamed to just `modernize-use-std-print`. This sticks with the convention of other modernize names and potentially in the future allows us

[PATCH] D147808: [clangd] Support non-trivial defaulted special member functions in Define Outline tweak

2023-05-04 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 519496. njames93 added a comment. Fix clang-format issues. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147808/new/ https://reviews.llvm.org/D147808 Files: clang-tools-extra/clangd/refactor/tweaks/De

[PATCH] D148423: [clangd] Support define outline tweak for non cross file edits

2023-05-04 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148423/new/ https://reviews.llvm.org/D148423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D140018: [clang-tidy] Support std::string_view in readability-redundant-string-cstr

2022-12-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp:56 +}; +typedef basic_string_view> string_view; } Can you add typedefs and tests for wstring_view and others Repository: rG LLVM Git

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

2022-12-15 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. I'm just a little uneasy with the description, this isn't "fixing" the linked issue, its more a workaround. The issue itself is also a bit of a non-issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D135405: fix handling of braced-init temporaries for modernize-use-emplace

2022-12-15 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp:1268 + + v2.emplace_back(NonTrivialWithVector()); + // CHECK-MESSAGES: :[[@LINE-1]

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

2022-01-17 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. How does this handle cases where a macro is Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106 + +bool hasBlankLines(const std::string &Text) { + enum class WhiteSpaceState { Comment at: cl

[PATCH] D117409: [clang-tidy] Move IncludeInserter into ClangTidyContext

2022-01-17 Thread Nathan James via Phabricator via cfe-commits
njames93 marked an inline comment as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/IncludeInserter.h:20 namespace tidy { -namespace utils { LegalizeAdulthood wrote: > njames93 wrote: > > LegalizeAdulthood wrote: > > > What's th

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

2022-01-17 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman. njames93 requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Build a fast factory

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

2022-01-17 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:45 + enum { + RED = 0xFF, + GREEN = 0x00FF00, Eugene.Zelenko wrote: > It'll be reasonable to support indentation. Option could be used to spec

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

2022-01-18 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:26-28 +// struct S { +// S(int x, unique_ptr y) : x(x), y(std::move(y)) {} +// }; avogelsgesang wrote: > ``` > // e.g. given `struct S{ int x;

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

2022-01-18 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 400893. njames93 added a comment. Herald added a subscriber: carlosgalvezp. Pre-allocate both vectors Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117529/new/ https://reviews.llvm.org/D117529 Files: clang-

[PATCH] D117129: [clang-tidy] Extract Class IncluderClangTidyCheck

2022-01-18 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D117129#3251577 , @aaron.ballman wrote: > In D117129#3246403 , @njames93 > wrote: > >> There is a hurdle to overcome with this, Currently each check can have its >> own include styl

[PATCH] D117593: [clang-tidy] Change google-explicit-constructor to ignore conversions and operators marked `explicit(false)`

2022-01-18 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: alexfh, aaron.ballman, LegalizeAdulthood. Herald added subscribers: carlosgalvezp, xazax.hun. njames93 requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. These `explicit(con

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

2022-01-18 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D117529#3251949 , @carlosgalvezp wrote: > Haven't looked much in detail so apologies if my comment is stupid - can't > CachedGlobList be used for this purpose? Should be a one-liner change I think. Not a stupid question, bu

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 401197. njames93 added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117529/new/ https://reviews.llvm.org/D117529 Files: clang-tools-extra/clang-tidy/ClangTidyModule.h clang-to

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 401209. njames93 added a comment. Remove now unnecessary changes in ClangTidyModule. Fix Cache miss reporting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117529/new/ https://reviews.llvm.org/D117529 Files:

[PATCH] D116875: [clang-tidy] Add performance-inefficient-array-traversal check

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 marked an inline comment as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/InefficientArrayTraversalCheck.h:25-26 +public: + InefficientArrayTraversalCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Na

[PATCH] D116875: [clang-tidy] Add performance-inefficient-array-traversal check

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 401234. njames93 marked an inline comment as done. njames93 added a comment. Use inheriting constructor for check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116875/new/ https://reviews.llvm.org/D116875 Fil

[PATCH] D116875: [clang-tidy] Add performance-inefficient-array-traversal check

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 401241. njames93 added a comment. Uploaded wrong diff Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116875/new/ https://reviews.llvm.org/D116875 Files: clang-tools-extra/clang-tidy/performance/CMakeLists.tx

[PATCH] D96286: [clangd][NFC] Change TidyProvider cache to use a linked list approach

2022-01-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96286/new/ https://reviews.llvm.org/D96286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D117939: [clang-tidy] Add more documentation about check development

2022-01-24 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. @aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117939/new/ https://re

[PATCH] D56303: [clang-tidy] Recognize labelled statements when simplifying boolean exprs

2022-01-24 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. A large portion of the changes, especially in the checks implementation file, appear to be NFC stylistic or formatting only fixes. While these changes are generally good, they shouldn't be a part of this patch. Instead they should be committed in their own NFC patch. T

[PATCH] D118016: [clang-tidy] Change code of SignalHandlerCheck (NFC).

2022-01-24 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:131 + if (const auto *TU = Result.Nodes.getNodeAs("TU")) { +// Call graph must be populated with the entire TU at the begin. +// (It is possible to add a single funct

[PATCH] D116875: [clang-tidy] Add performance-inefficient-array-traversal check

2022-01-25 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 403050. njames93 added a comment. Simplify check logic to only detect loop inits with 0 and incriment with 1. Functionality can be introduced with later patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11

[PATCH] D118224: [clang-tidy] bugprone-signal-handler improvements: display call chain

2022-01-26 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:192-193 diag(CallOrRef->getBeginLoc(), - "%0 may not be asynchronous-safe; " - "calling it from a signal handler may be dangerous") - << CalledFunction; -

[PATCH] D117593: [clang-tidy] Change google-explicit-constructor to ignore conversions and operators marked `explicit(false)`

2022-01-26 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D117593#3273562 , @aaron.ballman wrote: > Hmmm, this is a rule for checking a specific style guide. > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions > doesn't say that `explicit(false)` is a reasonab

[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)

2022-01-27 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:167 + const Expr *CallOrRef) { + const bool FunctionIsCalled = isa(CallOrRef); + This can probably just be inlined into

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

2022-01-27 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:29 + : ClangTidyCheck(Name, Context), +AnalyzeValues(Options.get("AnalyzeValues", 1)), +AnalyzeReferences(Options.get("AnalyzeReferences", 1)),

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

2022-01-28 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D118104#3279524 , @JesApp wrote: > I can (re)start a build manually. Let's see how that goes, before we start > doing workarounds like that. I think there's just some serious latency problems with the pre merge checks atm

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-01-28 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: aaron.ballman, alexfh, JonasToth, LegalizeAdulthood. Herald added subscribers: carlosgalvezp, xazax.hun. njames93 requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Create a

[PATCH] D118519: [clang-tidy] Organize the release notes a little better

2022-01-30 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:221-227 Removed checks ^^ Improvements to include-fixer - The improvements are... LegalizeAdulthood wrote: > salman-javed-nz wrote

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-01-30 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 404350. njames93 marked 5 inline comments as done. njames93 added a comment. Refactored implementation to not store state in ClangTidyContext. This will let external consumers of clang-tidy opt into the behaviour. As well as integrate the reporting to any s

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-01-31 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. @Eugene.Zelenko Do you think I should update release notes for this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118520/new/ https://reviews.llvm.org/D118520 ___ cfe-commits m

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-02-01 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 405111. njames93 added a comment. Added release notes. Remove AST dump of bound nodes, typically isn't very helpful Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118520/new/ https://reviews.llvm.org/D118520 F

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-02-02 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:239 + const ast_matchers::BoundNodes &Result) const { +if (CurrentlyProcessing) + CurrentlyProcessing->onProcessingCheckStart(CheckName, Re

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: klimek, aaron.ballman, LegalizeAdulthood. Herald added a subscriber: mgorny. njames93 requested review of this revision. Herald added projects: clang, clang-tools-extra. Herald added a subscriber: cfe-commits. Create a PrettyStackTraceEvent

[PATCH] D118520: [clang-tidy] Output currently processing check and nodes on crash

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 abandoned this revision. njames93 added a comment. I have decided to move this behaviour into the ast matcher directly, I feel in the long term, that should be a better home for this functionality. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I've reused the test cases from the old clang-tidy implementation of this, however this should eventually be moved into ASTMatchers tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120185/new/ https://reviews.llvm.org

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: .clang-tidy:1-10 +Checks: | + -* + clang-diagnostic-* + llvm-* + misc-* + -misc-unused-parameters + -misc-non-private-member-variables-in-classes This change doesn't belong in this patch. Commen

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Was there any real use case for making this polymorphic, The overhead for a virtual function call seems a little unnecessary IMO? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422

[PATCH] D120196: [clang-tidy][NFC] Remove Tristate from CachedGlobList

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: alexfh, aaron.ballman, whisperity, salman-javed-nz. Herald added subscribers: rnkovacs, xazax.hun. njames93 requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The tristate i

[PATCH] D120187: [clang-tidy] Allow newline characters as separators for checks in Clang-Tidy configurations

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp:100 + EXPECT_TRUE(!!Options); + EXPECT_EQ("-*,misc-*\nllvm-*\n-clang-*,\ngoogle-*\n", *Options->Checks); +} SimplyDanny wrote: > SimplyDanny wrote: > > n

[PATCH] D119562: Provide fine control of color in run-clang-tidy

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:240-249 + color_group = parser.add_mutually_exclusive_group() + color_group.add_argument('-use-color', action='store_true', dest='use_color', + help='Use co

[PATCH] D119562: Provide fine control of color in run-clang-tidy

2022-02-19 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Why have you removed execute perms form the scripts, can you add them back please `$ chmod +x run-clang-tidy.py'` Unless you are using windows in which case you will have to manually edit the diff Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D113422#992 , @carlosgalvezp wrote: > Nothing other than readability, `CachedGlobList` //is a// `GlobList` so it > made sense to implement it with standard, by-the-book inheritance. But no, > it's not used polymorphical

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 410139. njames93 added a comment. Moved TraceReporter into the MatchASTVisitor class Added some missing new lines Fixed patch not being based off a commit in trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D120185: [ASTMatchers] Output currently processing match and nodes on crash

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:1536 void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) { internal::MatchASTVisitor Visitor(&Matchers, Options); Visitor.set_active_ast_context(&Context); --

[PATCH] D119562: [clang-tidy] Provide fine control of color in run-clang-tidy

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:69 + val = val.lower() + if val in ['', 'true', '1']: +return True Didn't even

[PATCH] D119562: [clang-tidy] Provide fine control of color in run-clang-tidy

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGc57b8ca721dd: [clang-tidy] Provide fine control of color in run-clang-tidy (authored by kesyog, committed by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D118900: [ASTMatchers] Expand isInline matcher to VarDecl

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 410186. njames93 marked an inline comment as done. njames93 added a comment. Update description and release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118900/new/ https://reviews.llvm.org/D118900 Fi

[PATCH] D118927: [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

2022-02-20 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 410187. njames93 marked 2 inline comments as done. njames93 added a comment. Remove double negative Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118927/new/ https://reviews.llvm.org/D118927 Files: clang-t

[PATCH] D118900: [ASTMatchers] Expand isInline matcher to VarDecl

2022-02-23 Thread Nathan James via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc34d89818341: [ASTMatchers] Expand isInline matcher to VarDecl (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D120196: [clang-tidy][NFC] Remove Tristate from CachedGlobList

2022-02-23 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG79353f940cf4: [clang-tidy][NFC] Remove Tristate from CachedGlobList (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120196/new/ https:

<    14   15   16   17   18   19   20   21   >