This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Customize FileCheck prefix in check_clang-tidy.py to support multiple prefixes
ClosedPublic

Authored by zinovy.nis on Oct 7 2018, 11:49 AM.

Details

Summary

The patch extends the existing command line option -check-suffix (with alias -check-suffixes) to accept multiple comma-separated prefixes.

// RUN: %check_clang_tidy -check-suffix=USING-C,USING-D %s misc-unused-using-decls %t -- -- ...

or for the same

// RUN: %check_clang_tidy -check-suffixes=USING-C,USING-D %s misc-unused-using-decls %t -- -- ...

Diff Detail

Repository
rL LLVM

Event Timeline

zinovy.nis created this revision.Oct 7 2018, 11:49 AM

Thank you!

test/clang-tidy/check_clang_tidy.py
94–95 ↗(On Diff #168599)

How , can be allowed if that is the suffix separator?
Also, i think _ can be supported.

Eugene.Zelenko removed a subscriber: Restricted Project.

Removed "," from the list of allowed suffix characters.

zinovy.nis marked an inline comment as done.Oct 7 2018, 10:45 PM
zinovy.nis added inline comments.
test/clang-tidy/check_clang_tidy.py
94–95 ↗(On Diff #168599)

When discussing the previous patch Alex said on underscores:

I don't know whether it makes sense to endorse (or even allow) the use of underscore in the check suffix. The mix of underscores and dashes looks ugly and is prone to errors.

So it was intentionally.

zinovy.nis marked an inline comment as done.Oct 7 2018, 10:46 PM

The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?

test/clang-tidy/check_clang_tidy.py
112 ↗(On Diff #168609)

i think the whitespace in this line should be aligned with with the line above

alexfh accepted this revision.Oct 8 2018, 5:08 AM

Looks good with a comment.

test/clang-tidy/check_clang_tidy.py
52 ↗(On Diff #168609)

Maybe -check-suffixes to make it more obvious that multiple values are supported?

This revision is now accepted and ready to land.Oct 8 2018, 5:08 AM

The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?

Yes, all the existing tests pass successfully.

Looks good with a comment.

It can break the compatibility. May be it worth to add an alias "-check-suffixes" for this option?

  • Fixed diagnostics;
  • Interchangeable alias introduced: -check-suffixes for -check-suffix.
zinovy.nis marked 2 inline comments as done.Oct 8 2018, 11:45 AM
zinovy.nis added a comment.EditedOct 8 2018, 12:31 PM

I think it would make sense to migrate one test already, to use the new capability

This change was inspired by @lebedev.ri, so we expect him to provide us with such a test soon ;-)

zinovy.nis edited the summary of this revision. (Show Details)Oct 8 2018, 12:34 PM

I think it would make sense to migrate one test already, to use the new capability

This change was inspired by @lebedev.ri, so we expect him to provide us with such a test soon ;-)

This will hopefully deduplicate some check lines in D52771 test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

This revision was automatically updated to reflect the committed changes.

Hi,

It looks like this change has disabled FileCheck for all clang-tidy lit tests that don't use check-prefixes. So, they all trivially pass even if their CHECK... lines are wrong. An easy repro is just to randomly modify any CHECK line in a lit file (e.g. llvm/tools/clang/tools/extra/test/clang-tidy/readability-avoid-const-params-in-decls.cpp) and run ninja check-clang-tools.

In check_clang_tidy.py, if you add back (slightly modified) lines 93-95 to the else branch (line 132), it seems to fix the problem. For example, add:

has_check_fixes = check_fixes_prefixes[0] in input_text
has_check_messages = check_messages_prefixes[0] in input_text
has_check_notes = check_notes_prefixes[0] in input_text
zinovy.nis added a comment.EditedOct 12 2018, 2:07 AM

Hi,

It looks like this change has disabled FileCheck for all clang-tidy lit tests that don't use check-prefixes. So, they all trivially pass even if their CHECK... lines are wrong. An easy repro is just to randomly modify any CHECK line in a lit file (e.g. llvm/tools/clang/tools/extra/test/clang-tidy/readability-avoid-const-params-in-decls.cpp) and run ninja check-clang-tools.

In check_clang_tidy.py, if you add back (slightly modified) lines 93-95 to the else branch (line 132), it seems to fix the problem. For example, add:

has_check_fixes = check_fixes_prefixes[0] in input_text
has_check_messages = check_messages_prefixes[0] in input_text
has_check_notes = check_notes_prefixes[0] in input_text

Oh, thanks.
Here's a fix: https://reviews.llvm.org/D53194
Sorry for inconvenience.