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

dblaikie added a comment. In D80531#2353268 , @njames93 wrote: > In D80531#2352944 , @kkleine wrote: > >> @dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will >> "Stop and fail the build, if a

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

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

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

kkleine added a comment. @njames93 I could do that but the original Macros had were defined without a semicolon at the end and one had to add it manually. See this revision in which I replaced some occurrences of `DISALLOW_COPY_AND_ASSIGN`: eaebcbc67926a18befaa297f1778edde63baec9b

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

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

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

kkleine added a comment. @dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will "Stop and fail the build, if a compiler warning is triggered. Defaults to OFF." I wonder if any other test fails from clang tidy because. My test explicitly checks that a warning is issued (e.g. `

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

dblaikie added a comment. In D80531#2284388 , @kkleine wrote: > Hi @dblaikie . I did run `ninja check-all` and `/bin/llvm-lit -av > ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp` > on this very new rev

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

kkleine added a comment. Hi @dblaikie . I did run `ninja check-all` and `/bin/llvm-lit -av ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp` on this very new revision (abd70fb3983f342bc1c90f9c70a7b59790ad5206

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

kkleine added a comment. @dblaikie sorry for not getting to it for so long. I'm taking a look at the problem you've described now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 __

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

dblaikie added subscribers: sammccall, dblaikie. dblaikie added a comment. In D80531#2069637 , @kwk wrote: > In D80531#2069383 , @njames93 wrote: > >> LGTM with 2 small nits, but I'd still give a few days see if any

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGe636e6b79ac0: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro (authored by kwk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new

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

kwk added a comment. In D80531#2069383 , @njames93 wrote: > LGTM with 2 small nits, but I'd still give a few days see if anyone else > wants to weight in. I'm okay with this but I'd like to understand when it's time to wait for others? Only when a patc

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

kwk added a comment. Done. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:52 + + const std::string getMacroName() const { return MacroName; } + njames93 wrote: > Return by reference here. Yeah, that makes sense. St

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

kwk updated this revision to Diff 267960. kwk marked 4 inline comments as done. kwk added a comment. - Return macro name by reference - Add colon to paragraph showing code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llv

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

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. LGTM with 2 small nits, but I'd still give a few days see if anyone else wants to weight in. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAs

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

kwk marked 4 inline comments as done. kwk added a comment. Marked more comments as "done" after going through the code again. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:49 +R"cpp({0}(const {0} &) = delete; +const {0} &

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

kwk updated this revision to Diff 267021. kwk marked 9 inline comments as done. kwk added a comment. - Don't store SourceManager but get it from Preprocessor - Use getName instead of getNameStart - Remove FIXME - change warning message - doxygen: \a -> \p - Make MacroName private and add getter

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

kwk added a comment. @njames93 I've addressed all your comments and hope the patch is good to land now :) Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:25 + const SourceManager &SM) + : Check(Check), PP(PP), SM(SM) {}

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

njames93 added a comment. Few changes and nits then LGTM Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:25 + const SourceManager &SM) + : Check(Check), PP(PP), SM(SM) {} + nit: You don't need to store a

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

kwk added a comment. @njames93 and @Eugene.Zelenko do you guys think this patch is ready to be approved and land? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 _

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

kwk updated this revision to Diff 266438. kwk marked an inline comment as done. kwk added a comment. - fix link in doc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 Files: clang-tools-extra/clang-tidy/m

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

kwk added a comment. @Eugene.Zelenko thank you for your patience with me. I fixed the link. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 ___ cfe-commits mail

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41 + specification untouched. You might want to run the check :doc:`modernize-use-equals-delete + ` to get warnings for deleted + f

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

kwk updated this revision to Diff 266268. kwk added a comment. - Adjust link to documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

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

kwk marked an inline comment as done. kwk added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41 + specification untouched. You might want to run the check + `modernize-use-equals-delete `_ to get +

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

kwk marked 2 inline comments as done. kwk added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41 + specification untouched. You might want to run the check + `modernize-use-equals-delete `_ to get +

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41 + specification untouched. You might want to run the check + `modernize-use-equals-delete `_ to get + warnings for deleted functi

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

kwk updated this revision to Diff 266257. kwk marked 2 inline comments as done. kwk added a comment. - Refer to modernize-use-equals-delete for warning about deleted functions in private sections Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/

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

kwk added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:31 +* Notice that the migration example above leaves the ``private`` access + specification untouched. This opens room for improvement, yes I kno

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:31 +* Notice that the migration example above leaves the ``private`` access + specification untouched. This opens room for improvement

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

kwk added a comment. Thanks @njames93 and @Eugene.Zelenko for your review. Most of it, I addressed but for some I have comments. See inline. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:36 +// be the class name. +const

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

kwk updated this revision to Diff 266251. kwk marked 11 inline comments as done. kwk added a comment. - Avoid constructing string - CamelCase for vars - more readable snippets for documentation - Added empty line after header in documentation - Remove sentence from doc - Remove unimportant tests

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:23 + private: + - DISALLOW_COPY_AND_ASSIGN(Foo); + + Foo(const Foo &) = delete; I think two code snippets would b

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

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:32 + return; +if (std::string(Info->getNameStart()) != Check.MacroName) + return; ``` if (Info->getName() != Check.MacroNa

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

kwk marked 2 inline comments as done. kwk added a comment. @njames93 I've implemented everything you've mentioned. This simplified a lot. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41 + +.. option:: FinalizeWithSem

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

kwk updated this revision to Diff 266148. kwk marked 4 inline comments as done. kwk added a comment. - Wrap RUN lines - Use CamelCase variable name - Simplify access to options - Make check options const - Use two-line placeholder - Automate placement of semicolon at the end Repository: rG LLV

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

kwk planned changes to this revision. kwk added a comment. Thank you @njames93 for the review. I'm implementing what you wrote. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531

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

njames93 added a comment. In D80531#2053969 , @kwk wrote: > @Eugene.Zelenko thank you for the review. I've fixed all places that you've > mentioned. And have a question about one thing in particular. See inline. > > Do you by any chance know why `llvm-lit

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

kwk updated this revision to Diff 266101. kwk added a comment. - Move comment about FinalizeWithSemicolon into code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80531/new/ https://reviews.llvm.org/D80531 Files: clang-tools-extra/clang-tidy/mode

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

kwk added a comment. @Eugene.Zelenko thank you for the review. I've fixed all places that you've mentioned. And have a question about one thing in particular. See inline. Do you by any chance know why `llvm-lit` keeps complaining when I use `[[@LINE-1]]` in my tests as so many other tests do?

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

kwk updated this revision to Diff 266100. kwk marked 8 inline comments as done. kwk added a comment. - Make ReplaceDisallowCopyAndAssignMacroCheck only work in C++11 and above - Remove unrelated changes from clang-tools-extra/docs/clang-tidy/checks/list.rst - documentation simplification - Move c

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:46 + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { +return LangOpts.CPlusPlus; + } I think che

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

kwk created this revision. Herald added subscribers: cfe-commits, xazax.hun, mgorny. Herald added a project: clang. This check finds macro expansions of `DISALLOW_COPY_AND_ASSIGN(Type)` and replaces them with a deleted copy constructor and a deleted assignment operator. Before the `delete` keywor