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

Konrad Wilhelm Kleine via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 28 13:45:47 PDT 2020


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) {}
+
----------------
njames93 wrote:
> nit: You don't need to store a reference to the `SourceManager` as the `Preprocessor` holds a reference to it.
@njames93 thank you for letting me know. I passed it along because the ´ClangTidyCheck::registerPPCallbacks` that I override actually accepts both, `SourceManager` and the `PP`:

```
lang=c++
virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                                   Preprocessor *ModuleExpanderPP) {}
```

Other checks like `MakeSmartPtrCheck`, `PassByValueCheck` also do this similar to mine. 

Anyway, I've changed the code to your liking. 


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:52
+
+  const std::string MacroName;
+};
----------------
njames93 wrote:
> Make this private with a get function and I'll be happy. 
Done. But since the variable is `const` having it public shouldn't allow a caller do anything but read the macro name. But I understand that convention is king ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80531/new/

https://reviews.llvm.org/D80531





More information about the cfe-commits mailing list