Page MenuHomePhabricator

[clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix
ClosedPublic

Authored by lebedev.ri on Aug 18 2017, 1:27 PM.

Details

Summary

Currently, there is two configured prefixes: CHECK-FIXES and CHECK-MESSAGES
CHECK-MESSAGES checks that there are no test output lines with warning:|error:, which are not explicitly handled in lit tests.
However there does not seem to be a nice way to enforce for all the note: to be checked.
This was useful for me when developing D36836.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Aug 18 2017, 1:27 PM
aaron.ballman edited edge metadata.Aug 30 2017, 4:47 AM

Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle note in addition to warning and error? I'd prefer to keep the number of "magic" names as low as possible so I have to remember less stuff when writing or reviewing tests.

Note can be handled right now as well. E.g.

// CHECK-MESSAGES: [[@LINE-2]]:3: note: type deduction did not result in an owner

would the patch handle the codelocation correctly?

Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle note in addition to warning and error?

If i change CHECK-MESSAGES to also require all the notes to be checked, a *lot* of tests fail:

********************
Testing Time: 4.04s
********************
Failing Tests (165):
    Clang Tools :: clang-tidy/android-cloexec-accept.cpp
    Clang Tools :: clang-tidy/android-cloexec-accept4.cpp
    Clang Tools :: clang-tidy/android-cloexec-creat.cpp
    Clang Tools :: clang-tidy/android-cloexec-dup.cpp
    Clang Tools :: clang-tidy/android-cloexec-epoll-create.cpp
    Clang Tools :: clang-tidy/android-cloexec-epoll-create1.cpp
    Clang Tools :: clang-tidy/android-cloexec-fopen.cpp
    Clang Tools :: clang-tidy/android-cloexec-inotify-init.cpp
    Clang Tools :: clang-tidy/android-cloexec-inotify-init1.cpp
    Clang Tools :: clang-tidy/android-cloexec-memfd-create.cpp
    Clang Tools :: clang-tidy/android-cloexec-open.cpp
    Clang Tools :: clang-tidy/android-cloexec-socket.cpp
    Clang Tools :: clang-tidy/boost-use-to-string.cpp
    Clang Tools :: clang-tidy/bugprone-integer-division.cpp
    Clang Tools :: clang-tidy/bugprone-suspicious-memset-usage.cpp
    Clang Tools :: clang-tidy/bugprone-undefined-memory-manipulation.cpp
    Clang Tools :: clang-tidy/cert-dcl21-cpp.cpp
    Clang Tools :: clang-tidy/cert-oop11-cpp.cpp
    Clang Tools :: clang-tidy/clean-up-code.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-cstyle-cast.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init-delayed.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-vararg.cpp
    Clang Tools :: clang-tidy/deduplication.cpp
    Clang Tools :: clang-tidy/google-build-explicit-make-pair.cpp
    Clang Tools :: clang-tidy/google-explicit-constructor.cpp
    Clang Tools :: clang-tidy/google-readability-casting.c
    Clang Tools :: clang-tidy/google-readability-casting.cpp
    Clang Tools :: clang-tidy/google-readability-namespace-comments.cpp
    Clang Tools :: clang-tidy/google-readability-todo.cpp
    Clang Tools :: clang-tidy/llvm-include-order.cpp
    Clang Tools :: clang-tidy/llvm-twine-local.cpp
    Clang Tools :: clang-tidy/misc-argument-comment-gmock.cpp
    Clang Tools :: clang-tidy/misc-argument-comment-strict.cpp
    Clang Tools :: clang-tidy/misc-argument-comment.cpp
    Clang Tools :: clang-tidy/misc-assert-side-effect.cpp
    Clang Tools :: clang-tidy/misc-bool-pointer-implicit-conversion.cpp
    Clang Tools :: clang-tidy/misc-definitions-in-headers.hpp
    Clang Tools :: clang-tidy/misc-forwarding-reference-overload.cpp
    Clang Tools :: clang-tidy/misc-inaccurate-erase.cpp
    Clang Tools :: clang-tidy/misc-inefficient-algorithm.cpp
    Clang Tools :: clang-tidy/misc-lambda-function-name.cpp
    Clang Tools :: clang-tidy/misc-macro-parentheses.cpp
    Clang Tools :: clang-tidy/misc-macro-repeated-side-effects.c
    Clang Tools :: clang-tidy/misc-misplaced-const.c
    Clang Tools :: clang-tidy/misc-misplaced-const.cpp
    Clang Tools :: clang-tidy/misc-move-const-arg.cpp
    Clang Tools :: clang-tidy/misc-move-constructor-init.cpp
    Clang Tools :: clang-tidy/misc-move-forwarding-reference.cpp
    Clang Tools :: clang-tidy/misc-multiple-statement-macro.cpp
    Clang Tools :: clang-tidy/misc-static-assert.c
    Clang Tools :: clang-tidy/misc-static-assert.cpp
    Clang Tools :: clang-tidy/misc-string-compare.cpp
    Clang Tools :: clang-tidy/misc-string-constructor.cpp
    Clang Tools :: clang-tidy/misc-string-integer-assignment.cpp
    Clang Tools :: clang-tidy/misc-suspicious-semicolon.cpp
    Clang Tools :: clang-tidy/misc-suspicious-string-compare.c
    Clang Tools :: clang-tidy/misc-suspicious-string-compare.cpp
    Clang Tools :: clang-tidy/misc-swapped-arguments.cpp
    Clang Tools :: clang-tidy/misc-uniqueptr-reset-release.cpp
    Clang Tools :: clang-tidy/misc-unused-alias-decls.cpp
    Clang Tools :: clang-tidy/misc-unused-parameters.c
    Clang Tools :: clang-tidy/misc-unused-parameters.cpp
    Clang Tools :: clang-tidy/misc-unused-raii.cpp
    Clang Tools :: clang-tidy/misc-unused-using-decls.cpp
    Clang Tools :: clang-tidy/misc-virtual-near-miss.cpp
    Clang Tools :: clang-tidy/modernize-avoid-bind.cpp
    Clang Tools :: clang-tidy/modernize-deprecated-headers-cxx03.cpp
    Clang Tools :: clang-tidy/modernize-deprecated-headers-cxx11.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-basic.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-camelback.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-const.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-extra.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-lowercase.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-uppercase.cpp
    Clang Tools :: clang-tidy/modernize-make-shared-header.cpp
    Clang Tools :: clang-tidy/modernize-make-shared.cpp
    Clang Tools :: clang-tidy/modernize-make-unique-header.cpp
    Clang Tools :: clang-tidy/modernize-make-unique.cpp
    Clang Tools :: clang-tidy/modernize-pass-by-value-macro-header.cpp
    Clang Tools :: clang-tidy/modernize-pass-by-value.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal-delimiter.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal-replace-shorter.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal.cpp
    Clang Tools :: clang-tidy/modernize-redundant-void-arg-delayed.cpp
    Clang Tools :: clang-tidy/modernize-redundant-void-arg.cpp
    Clang Tools :: clang-tidy/modernize-replace-auto-ptr.cpp
    Clang Tools :: clang-tidy/modernize-return-braced-init-list.cpp
    Clang Tools :: clang-tidy/modernize-shrink-to-fit.cpp
    Clang Tools :: clang-tidy/modernize-unary-static-assert.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-cast-remove-stars.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-cast.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-iterator.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-new-remove-stars.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-new.cpp
    Clang Tools :: clang-tidy/modernize-use-bool-literals-ignore-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-bool-literals.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init-assignment.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init.cpp
    Clang Tools :: clang-tidy/modernize-use-emplace-ignore-implicit-constructors.cpp
    Clang Tools :: clang-tidy/modernize-use-emplace.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-default-copy.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-default.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-delete.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept-macro.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept-opt.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept.cpp
    Clang Tools :: clang-tidy/modernize-use-nullptr-basic.cpp
    Clang Tools :: clang-tidy/modernize-use-nullptr.cpp
    Clang Tools :: clang-tidy/modernize-use-override-ms.cpp
    Clang Tools :: clang-tidy/modernize-use-override.cpp
    Clang Tools :: clang-tidy/modernize-use-transparent-functors.cpp
    Clang Tools :: clang-tidy/modernize-use-using-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-using.cpp
    Clang Tools :: clang-tidy/nolint.cpp
    Clang Tools :: clang-tidy/nolintnextline.cpp
    Clang Tools :: clang-tidy/performance-faster-string-find.cpp
    Clang Tools :: clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
    Clang Tools :: clang-tidy/performance-for-range-copy.cpp
    Clang Tools :: clang-tidy/performance-inefficient-vector-operation.cpp
    Clang Tools :: clang-tidy/performance-type-promotion-in-math-fn.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-copy-initialization.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-delayed.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-header.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-incomplete-type.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param.cpp
    Clang Tools :: clang-tidy/readability-avoid-const-params-in-decls.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-few-lines.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-format.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-same-line.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-single-line.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements.cpp
    Clang Tools :: clang-tidy/readability-container-size-empty.cpp
    Clang Tools :: clang-tidy/readability-delete-null-pointer.cpp
    Clang Tools :: clang-tidy/readability-else-after-return.cpp
    Clang Tools :: clang-tidy/readability-function-size.cpp
    Clang Tools :: clang-tidy/readability-identifier-naming.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion-allow-in-conditions.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion-cxx98.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion.cpp
    Clang Tools :: clang-tidy/readability-inconsistent-declaration-parameter-name.cpp
    Clang Tools :: clang-tidy/readability-misplaced-array-index.cpp
    Clang Tools :: clang-tidy/readability-named-parameter.cpp
    Clang Tools :: clang-tidy/readability-non-const-parameter.cpp
    Clang Tools :: clang-tidy/readability-redundant-control-flow.cpp
    Clang Tools :: clang-tidy/readability-redundant-declaration-ignore-macros.cpp
    Clang Tools :: clang-tidy/readability-redundant-declaration.cpp
    Clang Tools :: clang-tidy/readability-redundant-function-ptr-dereference.cpp
    Clang Tools :: clang-tidy/readability-redundant-member-init.cpp
    Clang Tools :: clang-tidy/readability-redundant-smartptr-get.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-cstr-msvc.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-cstr.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-init-msvc.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-init.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr.cpp
    Clang Tools :: clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp
    Clang Tools :: clang-tidy/readability-static-accessed-through-instance.cpp
    Clang Tools :: clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
    Clang Tools :: clang-tidy/readability-uniqueptr-delete-release.cpp

  Expected Passes    : 392
  Unexpected Failures: 165

I'd prefer to keep the number of "magic" names as low as possible so I have to remember less stuff when writing or reviewing tests.

I do agree that it makes sense to keep it as low as possible, but also i see a clear logic between all thee current checks:
There are [currently] three types of output out of clang-tidy check that we care about: fix-it; error, warning, note.

  • The CHECK-FIXES is special, and only looks for fix-its.
  • It would make sense to always check all the error messages, thus rest of the magic does enforce -implicit-check-not=error
  • * CHECK-MESSAGES additionally enforces -implicit-check-not=warning
  • None of those two magics care about notes
  • * So i'm adding CHECK-NOTES, which is a CHECK-MESSAGES + -implicit-check-not=note

Note can be handled right now as well.

Yes. Adding this new prefix is about adding -implicit-check-not=notes.
I.e. if you use CHECK-MESSAGES, then it will only enforce you to have check-lines for all the error: and warning: the check outputs.
It does *not* enforce that all the note: are to be handled. CHECK-NOTES however would enforce that.

E.g.

// CHECK-MESSAGES: [[@LINE-2]]:3: note: type deduction did not result in an owner

would the patch handle the codelocation correctly?

I'm not sure what is the question to be honest. This does not change anything about codelocation handling

alright. i thought it would do something different, but the enforcement to handle all notes is a good thing. forget what i wrote :)

I do agree that it makes sense to keep it as low as possible, but also i see a clear logic between all thee current checks:

Thank you for the explanation. I think that makes sense to me, but I'd like to hear from @alexfh before accepting.

test/clang-tidy/check_clang_tidy.py
79–80 ↗(On Diff #113226)

I think the text should be rephrased to "CHECK-FIXES, CHECK-MESSAGES, or CHECK-NOTES not found in the input"

lebedev.ri marked an inline comment as done.

Reword the 'no magic found' error message.

aaron.ballman accepted this revision.Oct 21 2017, 8:24 AM

I am okay with this direction but would still like @alexfh to accept before you commit.

This revision is now accepted and ready to land.Oct 21 2017, 8:24 AM

@alexfh any thoughts on this one?

lebedev.ri changed the repository for this revision from rL LLVM to rCTE Clang Tools Extra.

Rebased.

I think you're set to commit this -- if @alexfh has concerns, they can be addressed post-commit.

I think you're set to commit this

Should i commit it though?
Until licensing concerns with D36836 are finally magically resolved and it is committed, there won't be any users, and since there is no tests for this, that would be a dead code with all the consequences.

  • if @alexfh has concerns, they can be addressed post-commit.

I think you're set to commit this

Should i commit it though?
Until licensing concerns with D36836 are finally magically resolved and it is committed, there won't be any users, and since there is no tests for this, that would be a dead code with all the consequences.

Hmm, I thought we had other tests that could make use of this functionality? However, I might be misremembering. Hold off on committing until there's test coverage, but it'd be good to look at existing tests to see if any of them can be modified.

It looks like some existing tests could benefit from this:

$ grep -R -l 'note: ' test/clang-tidy/
test/clang-tidy/bugprone-forward-declaration-namespace.cpp
test/clang-tidy/llvm-twine-local.cpp
test/clang-tidy/overlapping.cpp
test/clang-tidy/google-readability-nested-namespace-comments.cpp
test/clang-tidy/bugprone-suspicious-enum-usage-strict.cpp
test/clang-tidy/cert-static-object-exception.cpp
test/clang-tidy/fix.cpp
test/clang-tidy/google-readability-namespace-comments.cpp
test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
test/clang-tidy/performance-move-constructor-init.cpp
test/clang-tidy/fuchsia-default-arguments.cpp
test/clang-tidy/readability-misleading-indentation.cpp
test/clang-tidy/readability-inconsistent-declaration-parameter-name-macros.cpp
test/clang-tidy/fix-errors.cpp
test/clang-tidy/bugprone-use-after-move.cpp
test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp
test/clang-tidy/bugprone-argument-comment.cpp
test/clang-tidy/cppcoreguidelines-owning-memory.cpp
test/clang-tidy/readability-function-size.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp
test/clang-tidy/macros.cpp
test/clang-tidy/readability-container-size-empty.cpp
test/clang-tidy/Inputs/overlapping/o.h
test/clang-tidy/bugprone-forwarding-reference-overload.cpp

If you're interested in changing some of these tests to use CHECK-NOTES, there's no need to wait for a resolution on D36836 (which, unfortunately, may take really long). However, please update the relevant section of docs/clang-tidy/index.rst.

alexfh requested changes to this revision.Mar 15 2018, 7:00 AM
This revision now requires changes to proceed.Mar 15 2018, 7:00 AM
lebedev.ri updated this revision to Diff 154490.Jul 7 2018, 6:32 AM

Rebased, just to control bitrot, no changes.

alexfh requested changes to this revision.Jul 11 2018, 8:51 AM

As per the previous comment: I have no concerns as long as the documentation is updated and at least one existing test is changed to use this feature (see the list in the previous comment).

This revision now requires changes to proceed.Jul 11 2018, 8:51 AM

@lebedev.ri and @alexfh i would change the tests in https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this one?

For testing purposes, you could change a single line of hicpp-exception-baseclass.cpp to use the CHECK-NOTES. I do the rest :)

alexfh accepted this revision.Aug 9 2018, 3:46 PM

@lebedev.ri and @alexfh i would change the tests in https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this one?

For testing purposes, you could change a single line of hicpp-exception-baseclass.cpp to use the CHECK-NOTES. I do the rest :)

SGTM

This revision is now accepted and ready to land.Aug 9 2018, 3:46 PM

Rebase (ugh, bitrot), port test/clang-tidy/hicpp-exception-baseclass.cpp.

Add docs note.

This revision was automatically updated to reflect the committed changes.