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

Wed Sep 9 16:56:29 PDT 2020

dblaikie added subscribers: sammccall, dblaikie.
dblaikie added a comment.

In D80531#2069637 <https://reviews.llvm.org/D80531#2069637>, @kwk wrote:

> In D80531#2069383 <https://reviews.llvm.org/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 patch is approved or from the beginning of patch's life? I mean who looks at patches that are approved unless you filter for the amount of approvers? How many approvers must there be?

FWIW, I tend to - I have a queue of things to review, which doesn't have /much/ to do with whether they've already been reviewed. Sometimes, sometimes not - usually if I've decided it's worth looking at I'll still at least glance over it (enough to see if someone's asking for further consensus, etc).

While this is rather belated feedback, I've been seeing failures of this test consistently since it was committed, I think. Maybe there's something corrupted in my local client, but figured I'd mention/ask about it, in case it's useful. (+ @sammccall too):

  FAIL: Clang Tools :: clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp (18811 of 74169)
  ******************** TEST 'Clang Tools :: clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp' FAILED ********************
  Script:
  --
  : 'RUN: at line 1';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=DEFAULT /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp    modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp
  : 'RUN: at line 4';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=DIFFERENT-NAME /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp   modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp   -config="{CheckOptions: [    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,     value: MY_MACRO_NAME}]}"
  : 'RUN: at line 10';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=FINALIZE /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp   modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp   -config="{CheckOptions: [    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,     value: DISALLOW_COPY_AND_ASSIGN_FINALIZE}]}"
  : 'RUN: at line 16';   clang-tidy /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp -checks="-*,modernize-replace-disallow-copy-and-assign-macro"    -config="{CheckOptions: [     {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,      value: DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS}]}" | count 0
  : 'RUN: at line 21';   clang-tidy /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp -checks="-*,modernize-replace-disallow-copy-and-assign-macro"    -config="{CheckOptions: [     {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,      value: DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION}]}" | count 0
  --
  Exit Code: 1

  Command Output (stdout):
  --
  Running ['clang-tidy', '/usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp', '-fix', '--checks=-*,modernize-replace-disallow-copy-and-assign-macro', '-format-style=LLVM', '--', '-std=c++11', '-nostdinc++']...
  ------------------------ clang-tidy output -----------------------
  1 warning generated.
  /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp:33:3: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN' [modernize-replace-disallow-copy-and-assign-macro]
    DISALLOW_COPY_AND_ASSIGN(TestClass1);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp:33:3: note: FIX-IT applied suggested code changes
  clang-tidy applied 1 of 1 suggested fixes.

  ------------------------------------------------------------------
  ------------------------------ Fixes -----------------------------
  --- /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp.orig     2020-09-09 16:32:20.416366536 -0700
  +++ /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp  2020-09-09 16:32:20.476366112 -0700
  @@ -30,7 +30,8 @@

   class TestClass1 {
   private:
  -  DISALLOW_COPY_AND_ASSIGN(TestClass1);
  +  TestClass1(const TestClass1 &) = delete;
  +  const TestClass1 &operator=(const TestClass1 &) = delete;
   };
   //
   //

  <lots more similar failures>

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531