This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] NFC use CHECK-NOTES in tests for misc-misplaced-const
AbandonedPublic

Authored by JonasToth on Sep 29 2018, 7:40 AM.

Details

Event Timeline

JonasToth created this revision.Sep 29 2018, 7:40 AM
alexfh requested changes to this revision.Oct 1 2018, 5:05 AM
alexfh added inline comments.
test/clang-tidy/misc-misplaced-const.c
18

These notes are also just marginally useful and make it harder to change the test. I wonder whether an absolute line number would make more sense here. Or maybe just add a test for one of the notes and leave out the rest (and keep CHECK-MESSAGES)?

This revision now requires changes to proceed.Oct 1 2018, 5:05 AM
JonasToth added inline comments.Oct 1 2018, 5:35 AM
test/clang-tidy/misc-misplaced-const.c
18

Absolute line number makes sense. IMHO the tests should cover all generated diagnostics including the notes. Would you accept sticking with CHECK-NOTES but with absolute line numbers?

alexfh added inline comments.Oct 1 2018, 6:15 AM
test/clang-tidy/misc-misplaced-const.c
18

IMHO the tests should cover all generated diagnostics including the notes.

I wouldn't call this the most important goal. I'd say that tests should cover important aspects of the output, not every single character of it. Another useful feature is that tests should be easy to create, read, and change. In cases like this - where the benefit of the change is not obvious - I would leave the decision to the author of the check.

Aaron, WDYT?

JonasToth updated this revision to Diff 167919.Oct 2 2018, 4:06 AM
  • use absolute line number for note
JonasToth added inline comments.Oct 5 2018, 6:56 AM
test/clang-tidy/misc-misplaced-const.c
18

@aaron.ballman not sure if you overlooked that note

alexfh removed a reviewer: alexfh. alexfh added 1 blocking reviewer(s): aaron.ballman.Oct 8 2018, 5:09 AM
JonasToth abandoned this revision.Nov 27 2018, 3:44 AM

not so relevant and spams my view