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

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] D150126: [clang-tidy][WIP] Set traversal scope to prevent analysis in headers

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] D150254: [tidy] Fix possible use-after-free in IdentifierNamingCheck

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] D150254: [tidy] Fix possible use-after-free in IdentifierNamingCheck

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] D149280: [clang-tidy] Add modernize-printf-to-std-print check

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] D149723: [clang-tidy] Optimize performance of RenamerClangTidyCheck

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] D148423: [clangd] Support define outline tweak for non cross file edits

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] D147808: [clangd] Support non-trivial defaulted special member functions in Define Outline tweak

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] D149280: [clang-tidy] Add modernize-printf-to-std-print check

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] D148436: [clang-tidy] Fix crash in --dump-config

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] D148436: [clang-tidy] Fix crash in --dump-config

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

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] D148423: [clangd] Support define outline tweak for non cross file edits

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] D146842: [clang-tidy] Fix crash when handling invalid config values

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] D148318: [clang-tidy] Add `performance-avoid-endl` check

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] D141892: Implement modernize-use-constraints

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] D144828: [clang-tidy] Add misc-header-include-cycle check

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] D146842: [clang-tidy] Fix crash when handling invalid config values

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] D146842: [clang-tidy] Fix crash when handling invalid config values

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] D144828: [clang-tidy] Add misc-header-include-cycle check

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] D148423: [clangd] Support define outline tweak for non cross file edits

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] D148423: [clangd] Support define outline tweak for non cross file edits

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] D147808: [clangd] Support non-trivial defaulted special member functions in Define Outline tweak

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] D148318: [clang-tidy] Add `performance-avoid-endl` check

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 defaulted destructors in Define Outline tweak

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] D147808: [clangd] Support defaulted destructors in Define Outline tweak

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] D147802: [clangd] Handle destructors in DefineOutline tweak

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] D147802: [clangd] Handle destructors in DefineOutline tweak

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] D148110: [clang-tidy] Ctor arguments are sequenced if ctor call is written as list-initialization.

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] D147876: [clang-tidy] Support specifying checks as a list in the config file

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] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

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] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

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] D147955: [clang-tidy] Extend CheckOptions to support grouping checks options

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] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

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] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

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] D147876: [clang-tidy] Support specifying checks as a list in the config file

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] D147357: [clang-tidy] Add bugprone-optional-value-conversion check

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] D147955: [clang-tidy] Extend CheckOptions to support grouping checks options

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] D107294: [clang-tidy] adds warning to suggest users replace symbols with words

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] D144347: [clang-tidy] Add readability-forward-usage check

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] D147918: [clang-tidy] Added IgnoreVirtual option to misc-unused-parameters

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] D147924: [clang-tidy] Exclude template instantiations in modernize-use-override

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] D141892: Implement modernize-use-constraints

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] D147918: [clang-tidy] Added IgnoreVirtual option to misc-unused-parameters

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] D147876: [clang-tidy] Support specifying checks as a list in the config file

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] D147924: [clang-tidy] Exclude template instantiations in modernize-use-override

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 introducing checks as a list in the config file

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] D147808: [clangd] Support defaulted destructors in Define Outline tweak

njames93 created this revision. njames93 added reviewers: sammccall, kadircet. 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-extra. Som

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

njames93 updated this revision to Diff 511780. njames93 added a comment. Clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147802/new/ https://reviews.llvm.org/D147802 Files: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

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

njames93 created this revision. njames93 added reviewers: kadircet, sammccall. 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-extra. Fix

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. njames93 marked 2 inline comments as done. Closed by commit rG376168babb51: [clang-tidy] Add modernize-type-traits check (authored by njames9

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 marked 2 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:170 + +static constexpr char Bind[] = ""; + ccotter wrote: > NIT: should the bound node have some meaningful non-empt

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 updated this revision to Diff 511702. njames93 added a comment. Rebased and sorted release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137302/new/ https://reviews.llvm.org/D137302 Files: clang-tools-extra/clang-tidy/modernize/CM

[PATCH] D146875: [clang-tidy] Fix example provided by add_new_check.py

njames93 added a comment. In D146875#4229544 , @carlosgalvezp wrote: > I think easiest is to go for Option A) - this is how we normally write checks > nowadays. Maybe the discussion about removing support for `--fix-notes` can > be done in an RFC? Fix

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp:359 +void UseEarlyExitsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(translationUnitDecl(), this); +} PiotrZSL wrote: > This will tr

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

njames93 added a comment. In D130181#4213779 , @LegalizeAdulthood wrote: > I'm not certain that the result of the transformation is more "readable"; is > this check intended to aid conformance to a style guide? One of the driving factors of this is tha

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

njames93 added a comment. This is a great check that I've been meaning to work on for ages but never had time. It mostly looks good but there are a few issues I feel there is another issue that this can be very spammy with diagnostics(though clang-tidy is likely suppressing the duplicated ones).

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

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] D143342: [clang-tidy] Support std::format and std::print in readability-redundant-string-cstr

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

njames93 requested changes to this revision. njames93 added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:186-187 + traverse(TK_AsIs, + callExpr(anyOf(callee(fu

[PATCH] D142939: Fix handling of -> calls for modernize-use-emplace

njames93 accepted this revision. njames93 added a comment. LGTM Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:93 +// Matches if the node has canonical type matching any of the given names. +auto hasWantedType(const std::vector &TypeNames) { + return h

[PATCH] D142123: [clang-tidy] Add check to suggest use of #pragma once

njames93 added a comment. Also this goes for all your PRs, can you please upload them in future with full context `git diff -U99` should do it. Makes revewing changes easier and removes those `Context not available` message on phab CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1421

[PATCH] D142123: [clang-tidy] Add check to suggest use of #pragma once

njames93 added a comment. Can you just elaborate on what the registry is used for? Is the plan to support potentially dynamically loading `HeaderGuardStyle` classes. If thats the case, Then I'd argue we don't need the `HeaderGuardBase` check class, We can just create a `HeaderGuardCheck` class t

[PATCH] D142121: [clang-tidy] Refactor common functionality of HeaderGuardCheck into HeaderGuardBase

njames93 added a comment. What's the purpose behind this refactor, If its for 142123, then this should be blocked until a consensus is reached over there? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142121/new/ https://reviews.llvm.org/D142121

[PATCH] D142123: [clang-tidy] Add check to suggest use of #pragma once

njames93 added a comment. Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module). I feel this check would likely cause more harm than good. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142123/new/ https://revie

[PATCH] D141892: Implement modernize-use-constraints

njames93 added a comment. In D141892#4057722 , @ccotter wrote: > 2. replace the non `_v` templates to the `_v` variants `is_same` -> > `is_same_v` or the equivalent concept `same_as` See D137302 Repository: rG LLVM

[PATCH] D141838: [clang-tidy] fix a false positive of `cppcoreguidelines-avoid-non-const-global-variables`

njames93 added a comment. I'm not convinced this is the correct fix. The guildline states that tools should flag all variables declared at global or namespace scope. Therefore we shouldn't be flag

[PATCH] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

njames93 requested changes to this revision. njames93 added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp:173 +namespace n48 { +// CHECK-MESSAGES-DAG: :[[@LINE-2]]:1:

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 updated this revision to Diff 489347. njames93 added a comment. Extend test cases to check inline namespace, extension namespaces and alias namespaces Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137302/new/ https://reviews.llvm.org/D137

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 updated this revision to Diff 489342. njames93 marked an inline comment as done. njames93 added a comment. Remove unneeded static keyword Improved fix-it logic if there is a space between the template name and the `<` token Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D141770: [clang-tidy][NFC] Use C++17 nested namespaces in the clang-tidy folder

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. Assuming the pre-merge is happy, this LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141770/new/ https://reviews.llvm.org/D141770 _

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

njames93 added a comment. On the topic of Objective-C, they use `.m` and `.M` extensions, it may be worth pointing out that this list is case sensitive (I'm assuming that's going to be the case.) Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:151-152 Options

[PATCH] D141741: Add some subdirectories to the list of Abseil paths.

njames93 added a comment. Can you update some of the tests to make use of these new directories as well as update the release notes to mention these changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141741/new/ https://reviews.llvm.org/D14174

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 marked an inline comment as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:172 +void TypeTraitsCheck::registerMatchers(MatchFinder *Finder) { + static const ast_matchers::internal::VariadicDynCastAllOfMatcher

[PATCH] D141769: [clang-tidy][NFC] Use C++17 nested namespaces in add_new_check.py and rename_check.py

njames93 accepted this revision. njames93 added a comment. LGTM, just a couple points Comment at: clang-tools-extra/clang-tidy/rename_check.py:311-314 + # TODO: remove below replacement when all clang-tidy checks have been + # updated with C++17 nested namespaces.

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 updated this revision to Diff 489148. njames93 marked 6 inline comments as done. njames93 added a comment. Add IgnoreMacros options. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137302/new/ https://reviews.llvm.org/D1373

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 marked 6 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:17 + +static const llvm::StringSet<> ValueTraits = { +"alignment_of", carlosgalvezp wrote: > I'm guessing this li

[PATCH] D141463: [clang-tidy] Improve rename_check.py

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/rename_check.py:75 print("Renaming '%s' -> '%s'..." % (fileName, newFileName)) - os.rename(fileName, newFileName) + subprocess.check_call(["git", "mv", fileName, newFileName]) return newFileName

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. Just 2 more `CHECK-FIXES` lines need updating. FWIW the reason its good to be explicit is because FileCheck will start searching from the line where the previous CHECK-FIXES had and stop wh

[PATCH] D141569: [clang-tidy] Implement CppCoreGuideline F.18

njames93 added a comment. It'd be interesting to see how this handles variadic templates, I have a feeling if it isn't done correctly, there will be a lot of false positives. Can test be added to demonstrate the behaviour. What happens with code like this void foo(bar&& B) { std::move(B);

[PATCH] D141583: [clang-tidy][doc] Deprecate the AnalyzeTemporaryDtors option

njames93 accepted this revision. njames93 added a comment. In D141583#4047975 , @carlosgalvezp wrote: > Thanks for reviewing! I can also add that the actual code using the option > was removed around 5 years ago, so it's about time :) @njames93 Do you h

[PATCH] D138566: [clang-tidy][NFC] Make CheckFactories::CreateChecks* const

This revision was automatically updated to reflect the committed changes. Closed by commit rGb616811dde41: [clang-tidy][NFC] Make CheckFactories::CreateChecks* const (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138566/new/ h

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 updated this revision to Diff 488724. njames93 marked 2 inline comments as done. njames93 added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137302/new/ https://reviews.llvm.org/D137302 Files: clang-tools-e

[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

njames93 marked 2 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:140 +const internal::VariadicDynCastAllOfMatcher +dependentNameTypeLoc; // NOLINT(readability-identifier-naming) + ---

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

njames93 requested changes to this revision. njames93 added a comment. This revision now requires changes to proceed. Can you run this through clang format to make sure the pre merge is happy and address those nits Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/Av

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

njames93 added a comment. In D135405#4039820 , @BigPeet wrote: > Ping. > > (Sorry, it's my first time contributing to LLVM and I simply don't know what > happens next. Do I need to do anything? Or is it just waiting to get merged > at some point?) My a

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:34 +static StringRef TrimFirstChar(StringRef x) { return x.substr(1); } + This seems a bit of a needless change, if you want to remove the newline, just in date th

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions option

njames93 added a comment. You do raise a good point about duplication, Therefore does it make sense to also do the same with the IncludeStyle as lots of checks add new includes. Though there is a precedent to do away with LLVM/Google style, as clang-format should be responsible for reordering in

[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions option

njames93 added a comment. Can you explain the reasoning of why this approach is better than current approach where checks can use global options(`Options.getLocalOrGlobal("HeaderFileExtensions", utils::defaultHeaderFileExtensions())`) to access the same information? Some checks also have the g

[PATCH] D140968: [clang-tidy] Add check for passing the result of `std::string::c_str` to `strlen`

njames93 requested changes to this revision. njames93 added a comment. This revision now requires changes to proceed. I don't see the appear of this check as its a situation that I doubt ever appears in code bases. If there are open source code bases where this is a known problem can you please

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

njames93 added a comment. In D138655#4012557 , @Sockke wrote: > Improved the test file. Just a general workflow comment, can you please mark comments as done when they are addressed, or, if you feel the comment in unjustified, explain why. CHANGES SIN

[PATCH] D138655: [clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

njames93 added a comment. In D138655#4007822 , @carlosgalvezp wrote: > Now that I think a bit better about this I wonder - does it really make sense > that we increase the complexity of the check to cover for cases where code > does not compile? If it

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

njames93 accepted this revision. njames93 added a comment. LGTM, thank you for the patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140018/new/ https://reviews.llvm.org/D140018 ___ cfe-commits mailing

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

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] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

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] D140018: [clang-tidy] Support std::string_view in readability-redundant-string-cstr

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] D139113: Fix a couple additional cases in misc-use-anonymous-namespace only

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/UseAnonymousNamespaceCheck.cpp:44 + const SourceManager &SM = MatchedDecl->getASTContext().getSourceManager(); + if (!SM.isWrittenInMainFile(MatchedDecl->getLocation())) +return;

[PATCH] D138777: [clang-tidy] Add check bugprone-multiple-new-in-one-expression.

njames93 added a comment. I'm really not sure this is the right approach to solve the problem. If the concern here is leaking on exceptions, then the goalpost should be flagging all calls to global new and recommend a smart pointer factory instead. I think that check could even be made as a cpp

[PATCH] D90568: [clang] Add [is|set]Nested methods to NamespaceDecl

njames93 added a comment. In D90568#3949134 , @thakis wrote: > This breaks the build: http://45.33.8.238/linux/92294/step_4.txt > > Please take a look and revert for now if it takes a while to fix. Apologies it took 2 times, It should all be good now, lmk

[PATCH] D90568: [clang] Add [is|set]Nested methods to NamespaceDecl

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG15e76eed0c76: [clang] Add [is|set]Nested methods to NamespaceDecl (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D90568

  1   2   3   4   5   6   7   8   9   10   >