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

Mon Oct 26 11:00:01 PDT 2020

dblaikie added a comment.

In D80531#2353268 <https://reviews.llvm.org/D80531#2353268>, @njames93 wrote:

> In D80531#2352944 <https://reviews.llvm.org/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 from clang tidy because. My test explicitly checks that a warning is issued (e.g. `// CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:3: warning: prefer deleting`) and when the `LLVM_ENABLE_WERROR` propagates to this piece, it will indeed fail the build. But I wonder why this doesn't happen to the other clang-tidy checks. @njames93  @Eugene.Zelenko any ideas?
>
> The reason WERROR causes this to fail is the tests are generating a warning `Wextra-semi` for the `;` that appear after each macro usage in the class. clang-tidy would ignore this diagnostic as its set-up to ignore all warnings from clang.
> However when Werror is set, that warning is promoted to an error, and clang-tidy will emit all errors, regardless of their origin.
> This causes there to be extra diagnostics in the tests, though only in the tests where you invoke clang-tidy directly. Not sure why that obeys `LLVM_ENABLE_WERROR` likewise not sure why check_clang_tidy doesn't obey it.
> Anyway the simple fix is to just remove the `;` after the macro usages in the class

Oh, neat - thanks for identifying that! Probably be good for someone to figure out how to make the tests independent of the LLVM build flags like LLVM_ENABLE_WERROR in general.

In D80531#2353314 <https://reviews.llvm.org/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 of `DISALLOW_COPY_AND_ASSIGN`: eaebcbc67926a18befaa297f1778edde63baec9b <https://reviews.llvm.org/rGeaebcbc67926a18befaa297f1778edde63baec9b>. What do you suggest? Keep the semicolon to more closely match the original macros or remove it to make the test happy?

Alternatively, the DISALLOW_COPY_AND_ASSIGN macros could have a trivial implementation that makes the trailing semicolon not "extra", like the real macro? (I guess it could be any function declaration "void stub()" for instance)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531