Page MenuHomePhabricator

[Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output
ClosedPublic

Authored by vladimir.plyashkun on Jun 20 2017, 8:53 AM.

Details

Summary

To get properly integration Clang-Tidy with CLion IDE, next things were implemented:

  1. Preserve Message, FileOffset, FilePath in the clang-tidy output.
  2. Export all diagnostics, not just the ones with fixes
  3. Test-cases

Diff Detail

Repository
rL LLVM

Event Timeline

vladimir.plyashkun edited the summary of this revision. (Show Details)Jun 20 2017, 8:57 AM
vladimir.plyashkun edited the summary of this revision. (Show Details)
vladimir.plyashkun retitled this revision from Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output to [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output.Jun 20 2017, 9:10 AM
vladimir.plyashkun edited the summary of this revision. (Show Details)
vladimir.plyashkun added reviewers: fhahn, klimek.

updated CMakeLists.txt

correct revision with all changes

vladimir.plyashkun edited reviewers, added: ilya-biryukov, alexfh; removed: Restricted Project, fhahn.Jun 30 2017, 2:43 AM
vladimir.plyashkun removed a subscriber: Restricted Project.
alexfh requested changes to this revision.Jun 30 2017, 2:56 AM

Please upload a diff with full context (http://llvm.org/docs/Phabricator.html).

include/clang/Tooling/DiagnosticsYaml.h
86–95 ↗(On Diff #103353)

We should ensure clang-apply-fixes still works correctly in the presence of diagnostics with no fixes. It would be best to add a lit test for that.

unittests/Tooling/DiagnosticsYamlTest.cpp
171 ↗(On Diff #103353)

Please add a newline.

This revision now requires changes to proceed.Jun 30 2017, 2:56 AM
vladimir.plyashkun edited edge metadata.
  • fixed No newline at end of file problem
  • provided test-case to check that diagnostics with no fixes will be applied correctly
alexfh requested changes to this revision.Jul 4 2017, 6:54 AM
alexfh added inline comments.
unittests/Tooling/DiagnosticsYamlTest.cpp
51 ↗(On Diff #105078)

EXPECT_STREQ is more appropriate here. ASSERT_* family of macros terminates the test, and thus should only be used to verify the invariants that are absolutely required to continue the execution of the test, which is not the case here.

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
18 ↗(On Diff #105078)

I'd remove const from DiagnosticName, since a StringRef doesn't allow to modify the string it points to (and marking by value arguments const is not common in LLVM). However, Message and Replacements allow to modify the referenced objects and thus should be marked const.

Also consider inlining this method, it doesn't buy you much.

23 ↗(On Diff #105078)

Will {} work instead of EmptyNotes?

23 ↗(On Diff #105078)

tooling:: is not needed due to the using directive above. Same below.

54–56 ↗(On Diff #105078)

I'd remove the success variable and just call EXPECT_TRUE(merge...)

This revision now requires changes to proceed.Jul 4 2017, 6:54 AM
unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
23 ↗(On Diff #105078)

Unfortunately, no.
Constructor expects non-const reference SmallVector<DiagnosticMessage, 1> &Notes, so i can't pass rvalue directly to it.

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
18 ↗(On Diff #105078)

Same for here.
Constructor for Diagnostic class requires non-const references for Message and Replacements arguments. So, it's invalid to mark them as const.

vladimir.plyashkun edited edge metadata.
  • use EXPECT_* instead of ASSERT_* where it's possible
  • Diagnostic constructor now takes const references for it's arguments
  • removed extra namespace qualifiers
alexfh added inline comments.Jul 5 2017, 4:47 AM
unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
23 ↗(On Diff #105078)

I overlooked this when committing someone's patch. Fixed in r307143. Now you can add const and probably also use {}.

alexfh requested changes to this revision.Jul 5 2017, 4:48 AM
This revision now requires changes to proceed.Jul 5 2017, 4:48 AM
fhahn removed a subscriber: fhahn.Jul 5 2017, 4:52 AM
vladimir.plyashkun edited edge metadata.
  • marked some arguments as const
  • use {} instead of explicit variable declarations
unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
23 ↗(On Diff #105078)

Thanks, fixed it.

alexfh added a comment.Jul 5 2017, 6:57 AM

A few more nits.

unittests/Tooling/DiagnosticsYamlTest.cpp
39 ↗(On Diff #105260)

The assignment form of initialization is clearer for containers (and actually any type for which the initialization means "this variable is whatever is on the right side of the assignment" as opposed to "this variable is initialized by a constructor call taking these parameters").

60 ↗(On Diff #105260)

Why not EXPECT_EQ (and remove .c_str() from the second argument)?

unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
27 ↗(On Diff #105260)

I believe, this can be expressed a bit less verbosely, e.g. TUs.push_back({ MainSourceFile, {Diagnostic(...)}});.

vladimir.plyashkun added a comment.EditedJul 5 2017, 7:23 AM

Thanks, Alex for your suggestions.
I agree with your remarks, but i thought it's only personal style preferences.
I'll fix it soon.

vladimir.plyashkun edited the summary of this revision. (Show Details)
  • made code less verbose
  • used assignment initialization form
  • removed extra .c_str() call
alexfh accepted this revision.Jul 7 2017, 2:14 AM

Looks good. Thanks!

This revision is now accepted and ready to land.Jul 7 2017, 2:14 AM

I tried landing the patch for you, but it doesn't apply cleanly. One reason is that it contains changes to both cfe and clang-tools-extra repos. But even when I apply the patch to the two directories it breaks a bunch of clang-apply-replacements tests:

Failing Tests (4):
    Clang Tools :: clang-apply-replacements/basic.cpp
    Clang Tools :: clang-apply-replacements/conflict.cpp
    Clang Tools :: clang-apply-replacements/crlf.cpp
    Clang Tools :: clang-apply-replacements/format.cpp

Please rebase the patch and split it in two (upload a second differential revision for the clang-tools-extra part).

  1. split revision into two
  2. fix failing tests

I've splitted single revision into two different revisions:
https://reviews.llvm.org/D34404
https://reviews.llvm.org/D35349
Also, i fixed failing test-cases in clang-apply-replacements due to changes in output format (so they shouldn't be a problem anymore)

alexfh added inline comments.Jul 14 2017, 3:38 AM
unittests/Tooling/DiagnosticsYamlTest.cpp
35–45 ↗(On Diff #106402)

Note: there's a bunch of trailing whitespace here and the formatting is off in a few places. git-clang-format fixed both issues, but it would be nice if you set up your editor properly or run clang-format yourself next time.

Same for the other CL, but there I had to also clean up trailing whitespace from yaml files as well.

This revision was automatically updated to reflect the committed changes.