[PATCH] D137968: [clang-tidy] Ignore overriden methods in `readability-const-return-type`.

2022-11-14 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. Lg, but can you address that one nit before landing Comment at: clang-tools-extra/docs/ReleaseNotes.rst:169 +- Change the behavior of :doc:`readability-const-return-type + ` to not Can this be moved

[PATCH] D138031: Add clang-tidy check for missing move constructors

2022-11-15 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. What is the purpose of this check when we have cppcoreguidelines-special-member-functions . Granted that check won't emit replacements, however the replacements that this

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

2023-02-01 Thread Nathan James via Phabricator via cfe-commits
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] D143342: [clang-tidy] Support std::format and std::print in readability-redundant-string-cstr

2023-02-06 Thread Nathan James via Phabricator via cfe-commits
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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D138566: [clang-tidy][NFC] Make CheckFactories::CreateChecks* const

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D141583: [clang-tidy][doc] Deprecate the AnalyzeTemporaryDtors option

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D141569: [clang-tidy] Implement CppCoreGuideline F.18

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D141133: [clang-tidy] Implement CppCoreGuideline F.54

2023-01-12 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. 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] D141463: [clang-tidy] Improve rename_check.py

2023-01-12 Thread Nathan James via Phabricator via cfe-commits
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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-13 Thread Nathan James via Phabricator via cfe-commits
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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-13 Thread Nathan James via Phabricator via cfe-commits
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] D141769: [clang-tidy][NFC] Use C++17 nested namespaces in add_new_check.py and rename_check.py

2023-01-14 Thread Nathan James via Phabricator via cfe-commits
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

2023-01-14 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/modernize/TypeTraitsCheck.cpp:172 +void TypeTraitsCheck::registerMatchers(MatchFinder *Finder) { + static const ast_matchers::internal::VariadicDynCastAllOfMatcher

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

2023-01-14 Thread Nathan James via Phabricator via cfe-commits
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] D141000: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

2023-01-14 Thread Nathan James via Phabricator via cfe-commits
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] D141770: [clang-tidy][NFC] Use C++17 nested namespaces in the clang-tidy folder

2023-01-14 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. 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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-15 Thread Nathan James via Phabricator via cfe-commits
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] D137302: [clang-tidy] Add modernize-type-traits check

2023-01-15 Thread Nathan James via Phabricator via cfe-commits
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] D141787: [clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

2023-01-15 Thread Nathan James via Phabricator via cfe-commits
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] D141838: [clang-tidy] fix a false positive of `cppcoreguidelines-avoid-non-const-global-variables`

2023-01-16 Thread Nathan James via Phabricator via cfe-commits
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] D141892: Implement modernize-use-constraints

2023-01-17 Thread Nathan James via Phabricator via cfe-commits
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] D142123: [clang-tidy] Add check to suggest use of #pragma once

2023-01-19 Thread Nathan James via Phabricator via cfe-commits
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] D142121: [clang-tidy] Refactor common functionality of HeaderGuardCheck into HeaderGuardBase

2023-01-19 Thread Nathan James via Phabricator via cfe-commits
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

2023-01-21 Thread Nathan James via Phabricator via cfe-commits
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] D142123: [clang-tidy] Add check to suggest use of #pragma once

2023-01-21 Thread Nathan James via Phabricator via cfe-commits
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] D125885: [clang-tidy] bugprone-argument-comment: Ignore calls to user-defined literals

2022-06-20 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D125885#3595301 , @jspam wrote: > Would someone be able to commit this for me, as I do not have the necessary > rights? Thanks I can for you, what name and email address should I use? Repository: rG LLVM Github Monorepo

[PATCH] D125885: [clang-tidy] bugprone-argument-comment: Ignore calls to user-defined literals

2022-06-20 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG541a50e20702: [clang-tidy] bugprone-argument-comment: Ignore calls to user-defined literals (authored by jspam, committed by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D128204: [clangd] Add fix-it for inserting IWYU pragma: keep

2022-06-20 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: sammccall, kadircet, kbobyrev. Herald added subscribers: usaxena95, arphaman. Herald added a project: All. njames93 requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: cla

[PATCH] D126859: [clangd] Validate clang-tidy CheckOptions in clangd config

2022-06-20 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 438388. njames93 added a comment. Reuse implementation from D127446 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126859/new/ https://reviews.llvm.org/D126859 Files: clang

[PATCH] D128204: [clangd] Add fix-it for inserting IWYU pragma: keep

2022-06-21 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I had another idea about offering the export pragma when in a header file but I don't know if that's going too far In D128204#3599286 , @kadircet wrote: > Hence i'd like to hear a little bit more about what kind of false positi

[PATCH] D128204: [clangd] Add fix-it for inserting IWYU pragma: keep

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D128204#3600973 , @kadircet wrote: >> One of the main issues I have is due to templates not being instantiated >> which can result in symbols that are actually used not being picked up as >> the template instantiation that u

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 marked 4 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:263 +Check the config files to ensure each check and +option is recognised. +)"), aaron.ballman wrote: > :) =

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 438953. njames93 marked an inline comment as done. njames93 added a comment. Address reviewer comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127446/new/ https://reviews.llvm.org/D127446 Files: clan

[PATCH] D126859: [clangd] Validate clang-tidy CheckOptions in clangd config

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D126859#3599340 , @kadircet wrote: > I also agree with the typo correction verdict. In theory there'll be two > cases: > > - typo correction helps, in which case it'll be obvious from the warning > itself. > - typo correcti

[PATCH] D128314: [Clang-tidy] Fixing bugs in clang-tidy infinite-loop checker

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Can this patch be split in two, it seems strange to be fixing 2 unrelated bugs in one patch. One fix for the ObjC nodes and another for the patch for the static local variables. Also please can you run git clang-format over this patch. Comment at: c

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: aaron.ballman, alexfh. Herald added subscribers: carlosgalvezp, arphaman, xazax.hun. Herald added a project: All. njames93 requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a project: clang-tools

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp:12 + +// CHECK-VERIFY-DAG: command-line option '-config': warning: Unknown Check 'readability-else-after-ret'; did you mean 'readability-else-after-return' +// CHECK-V

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added subscribers: kadircet, sammccall. njames93 added a comment. Before this lands @sammccall @kadircet Do you have any concerns about this API for getting the options not building a static instance here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D126859: [clangd] Validate clang-tidy CheckOptions in clangd config

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 439004. njames93 added a comment. Remove typo correction support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126859/new/ https://reviews.llvm.org/D126859 Files: clang-tools-extra/clangd/ConfigCompile.cpp

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 439018. njames93 added a comment. Herald added subscribers: usaxena95, kadircet. Update some test files using new syntax. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128337/new/ https://reviews.llvm.org/D128

[PATCH] D128355: [clangd][NFC] Unify ancestor logic for config and tidy provider.

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: sammccall, kadircet. Herald added subscribers: usaxena95, 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-e

[PATCH] D128072: [clang-tidy] Organize test files into subdirectories by module (NFC)

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Just noticed something nice about this, cmake targets are generated for each module Should speed up development as you now only need to run the lit tests for the module that you are working on `check-clang-extra-clang-tidy-checkers-misc` etc Repository: rG LLVM Gith

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Thank you for this, it has been annoying me for a while. I'll give a tentative LG, but I'm no expert in this area so see what everyone else says first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127807/new/ https://rev

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D128337#3602694 , @aaron.ballman wrote: > I like the changes -- this is a much nicer syntax for specifying > configuration options! > >> The only observable differences are support for the new syntax and >> -dump=config wil

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 439139. njames93 marked an inline comment as done. njames93 added a comment. Rebased and addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128337/new/ https://reviews.llvm.org/D128337 Files:

[PATCH] D128379: [clangd] Change the url for clang-tidy check documentation

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: sammccall, kadircet. Herald added subscribers: usaxena95, 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-e

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:26 +// of the left and right statements. +const Expr *LHSExpr = llvm::dyn_cast(LHS); +const Expr *RHSExpr = llvm::dyn_cast(RHS); Eugene.Zelenko wrot

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

2022-06-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp:64-68 + unless(hasType(isVolatileQualified())), // non-volatile + unless(isTemplateVariable()), // non-templat

[PATCH] D128379: [clangd] Change the url for clang-tidy check documentation

2022-06-23 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:928 +std::tie(Module, Check) = Name.split('-'); +assert(!Module.empty() && !Check.empty()); +return {("https://clang.llvm.org/extra/clang-tidy/checks/"; + Module + "/" + -

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-23 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG5ca68d5845c0: [clang-tidy] Add `-verify-config` command line argument (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127446/new/ http

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-23 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 439478. njames93 added a comment. Rebase to check CI Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128337/new/ https://reviews.llvm.org/D128337 Files: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp cla

[PATCH] D128337: [clang-tidy] Extend spelling for CheckOptions

2022-06-23 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGfbf611ed2a76: [clang-tidy] Extend spelling for CheckOptions (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128337/new/ https://review

[PATCH] D128379: [clangd] Change the url for clang-tidy check documentation

2022-06-24 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Pointing the documentation links based on build configuration and version definitely makes sense as evolving checks could easily confuse users. Guess the idea is snapshot builds will use the in progress release notes, and release builds can point to the actual release d

[PATCH] D126162: [clang-tidy] Extend SimplifyBooleanExpr demorgan support.

2022-05-25 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 431945. njames93 added a comment. Add note to docs that this new option has no effect if SimplifyDeMorgan is false. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126162/new/ https://reviews.llvm.org/D126162

[PATCH] D126162: [clang-tidy] Extend SimplifyBooleanExpr demorgan support.

2022-05-25 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 rGf3c1d281767a: [clang-tidy] Extend SimplifyBooleanExpr demorgan support. (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D126859: [clangd] Validate clang-tidy CheckOptions in clangd config

2022-06-02 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: sammccall, kadircet. Herald added subscribers: usaxena95, 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-e

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

2022-06-02 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Can I ask what the motivation is for this change? You may want to check the actual header files for the checks. A lot of them have a comment saying. /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-reserved-iden

[PATCH] D126780: [clang-tidy] `bugprone-use-after-move`: Fix handling of moves in lambda captures

2022-06-02 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, But please add a note to ReleaseNotes before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126780/new/ https://reviews.llvm.o

[PATCH] D126853: [clang-tidy] `bugprone-use-after-move`: Don't warn on self-moves.

2022-06-03 Thread Nathan James via Phabricator via cfe-commits
njames93 accepted this revision. njames93 added a comment. LGTM. I feel that this case should produce a warning akin to the no self assignment diagnostics, obviously nothing to do with this check though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

2022-06-08 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 a couple points but not essential. Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:184-187 + if (ScopesCache.find(LocalScope) == Scope

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

2022-06-08 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h:35 +TransformPointersAsValues( +Options.get("TransformPointersAsValues", false)) {} + JonasToth wrote: > njames93 wrote: > > It may be worth

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

2022-06-08 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:558 + S1() {} + // CHECK-MESSAGES-NOT: warning: + union { This line is redundant and likely to cause unintended test failures, sa

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

2022-06-08 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D126495#3568998 , @LegalizeAdulthood wrote: > Gentle ping My previous point about the links in the header files not being updated hasn't been addressed. It would also be nice if there was a redirect that would dynamically

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

2022-06-09 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D126495#3571403 , @LegalizeAdulthood wrote: > In D126495#3570767 , @aaron.ballman > wrote: > >> In D126495#3569863 , >> @LegalizeAdulthood

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-09 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: alexfh, aaron.ballman. Herald added subscribers: carlosgalvezp, arphaman, 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.

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

2022-06-10 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D127446#3572491 , @carlosgalvezp wrote: > This sounds like great functionality, surely saving a lot of headaches! Any > reason why we wouldn't want this active by default? I'd personally even go > one step further and make

[PATCH] D126859: [clangd] Validate clang-tidy CheckOptions in clangd config

2022-06-10 Thread Nathan James via Phabricator via cfe-commits
njames93 planned changes to this revision. njames93 added a comment. Gonna wait for the infrastructure from D127446 to land first, can reuse most of that implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

2022-11-20 Thread Nathan James via Phabricator via cfe-commits
njames93 added a subscriber: sammccall. njames93 added a comment. @sammccall I have a feeling you're gonna want to examine this checks feasibility in clangd. Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:79 + +static auto findDeclRefBl

[PATCH] D138505: [clangd] Don't run slow clang-tidy checks

2022-11-23 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D138505#3944285 , @sammccall wrote: > Ideas on testing welcome. Does it make sense to rely on the fact that > `misc-const-correctness` is always slow? :-D I'd say it doesn't, if the check is ever updated in a way to be more

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

2022-11-23 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: aaron.ballman, gribozavr2, JonasToth. Herald added subscribers: carlosgalvezp, 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-comm

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

2022-11-23 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 477467. njames93 added a comment. Hopefully fixed line endings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90568/new/ https://reviews.llvm.org/D90568 Files: clang/include/clang/AST/Decl.h clang/include/

[PATCH] D138583: Create unused non-trivially-destructible check in clang-tidy

2022-11-24 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. Clang already has a warning `-Wunused-variable` that is designed for this specific purpose. So unless this is bringing anything enhanced functionality I don't see the need for th

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

2022-11-24 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 rG15e76eed0c76: [clang] Add [is|set]Nested methods to NamespaceDecl (authored by njames93). Changed prior to commit: https://reviews.llvm.org/D90568

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

2022-11-24 Thread Nathan James via Phabricator via cfe-commits
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] D138777: [clang-tidy] Add check bugprone-multiple-new-in-one-expression.

2022-11-28 Thread Nathan James via Phabricator via cfe-commits
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] D139113: Fix a couple additional cases in misc-use-anonymous-namespace only

2022-12-01 Thread Nathan James via Phabricator via cfe-commits
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] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

2020-03-21 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I'm not hugely familiar with the abseil library, but from what I can see `absl::StartsWith` takes `absl::string_view` as args. Therefore surely it makes sense to handle the 3rd arg for `str::find` by explicitly constructing the `absl::string_view` from the pointer and

[PATCH] D76541: [clang-tidy][NFC] Add missing check group docs and order entries

2020-03-21 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. Forgive me if I'm wrong, but these kinds of changes typically don't require a review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76541/new/ https://reviews.llvm.org/D76541 ___

[PATCH] D76549: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs

2020-03-21 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: aaron.ballman, gribozavr2, alexfh. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. Previously if a type was accessed with a qualifier, RenamerClangTidy wouldn't rename the TypeLoc, this patch addresses this

[PATCH] D76545: [clang-tidy] Add a new check group 'experimental-'

2020-03-21 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I'm not sold on this being necessary as there is nothing against simply putting the check in the right module to begin with, then changing the registerCheck call to `experimenal-` as well as the docs. Could even do some fancy trickery with the python scripts to add sup

[PATCH] D76549: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs

2020-03-22 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 251910. njames93 added a comment. - Address `auto` and add test cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76549/new/ https://reviews.llvm.org/D76549 Files: clang-tools-extra/clang-tidy/utils/Rena

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

2020-03-22 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D76054#1935533 , @ymandel wrote: > Thanks for expanding the description, including the helpful example. I'm not > sure, though, that this is the "right" behavior, at least not always. Worse, > I'm not sure there is a single

[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
njames93 marked 2 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:154 + llvm::errs() << "warning: Invalid case style '" << CaseValue + << "' for readability-identifier-

[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. njames93 added reviewers: aaron.ballman, alexfh. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. njames93 marked 2 inline comments as done. njames93 added a project: clang-tools-extra. njames93 added a comment. I'm not too sure how m

[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. I'm not too sure how many other checks are like this, but I feel that this functionality could maybe be brought out of this check and into the Options to let more checks that take enum like configurations to use it. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments. Comment at: clang-tools-extra/test/clang-apply-replacements/identical2.cpp:3 +// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical2/identical2.cpp > %T/Inputs/identical2/identical2.cpp +// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" %S/Inputs/ide

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 252017. njames93 marked 5 inline comments as done. njames93 added a comment. - Address reviewer comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76054/new/ https://reviews.llvm.org/D76054 Files: clang

[PATCH] D76549: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs

2020-03-23 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG7693a9b93140: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76549/new/

[PATCH] D73541: [Clang] Added method getTokenLocations to StringLiteral

2020-03-23 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/D73541/new/ https://reviews.llvm.org/D73541 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming

2020-03-24 Thread Nathan James via Phabricator via cfe-commits
njames93 marked 3 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:149-151 +auto CaseOptionName = (Name + "Case").str(); +auto CaseValue = Options.get(CaseOptionName, ""); +auto co

[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. This check should only worry about the first declaration of the function, any redecls or the definition could appear outside the namespace like this: namespace blah{ void foo(); } void blah::foo(); void blah::foo(){} There are some missing test cases I'd

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG6538b4393dc3: [clang-apply-replacements] No longer deduplucates replacements from the same TU (authored by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D76761 Files: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp clang-tools-extra/test/clang-ti

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D76761#1941070 , @gribozavr2 wrote: > Could you provide more information about why these null checks are needed in > this case? Well the fact it was crashing without those null checks was the reason they were added. I'm not

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D76761#1941476 , @gribozavr2 wrote: > Right -- what I meant is a more detailed description of why, for example, > `tryGetCallExprAncestorForCxxConstructExpr` can't find the `CallExpr` in this > case -- is it not there, or doe

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

2020-03-25 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 252646. njames93 added a comment. - Fixed by replacing cumbersome code with the arguably proper solution Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76761/new/ https://reviews.llvm.org/D76761 Files: clang

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

2020-03-26 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. If you want to make it a general check, you should consider making the default module options set the correct namespace RequiredNamespace ClangTidyOptions getModuleOptions() override { ClangTidyOptions Options; Options.CheckOptions["llvmlibc-implementation-in

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

2020-03-27 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment. In D76496#1947059 , @niko wrote: > In D76496#1935127 , @njames93 wrote: > > > I'm not hugely familiar with the abseil library, but from what I can see > > `absl::StartsWith` takes `absl::s

<    10   11   12   13   14   15   16   17   18   19   >