Page MenuHomePhabricator

Added CheckName field to YAML report
AbandonedPublic

Authored by alexfh on Jan 14 2016, 5:01 AM.

Diff Detail

Event Timeline

Elijah_Th retitled this revision from to Added CheckName field to YAML report.
Elijah_Th updated this object.
alexfh edited edge metadata.Jan 14 2016, 5:14 AM

I'm not sure tooling::Replacement is the best place to store check name. Maybe create a separate wrapper class and serialize it instead (clang-apply-replacements will have to be changed to support this format as well). This could be ClangTidyDiagnostic or just Diagnostic, and we could also store the message and other useful information in it.

tools/clang/include/clang/Tooling/Core/Replacement.h
93 ↗(On Diff #44862)

Please clang-format the code.

tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
80 ↗(On Diff #44862)

Doesn't it compile without StringRef() around Error.CheckName?

Elijah_Th edited edge metadata.
  • small format changes
  • removed unnecessary StringRef

Thanks for addressing the comments. However, the main concern is this:

I'm not sure tooling::Replacement is the best place to store check name. Maybe create a separate wrapper class and serialize it instead (clang-apply-replacements will have to be changed to support this format as well). This could be ClangTidyDiagnostic or just Diagnostic, and we could also store the message and other useful information in it.

Elijah_Th marked 2 inline comments as done.Jan 18 2016, 9:19 AM

What kind of wrapper should it be?
I was thinking of this kind:

class ExtendedReplacement : public Replacement {
public:
  ExtendedReplacement(StringRef CheckName, Replacement &R);

  StringRef getCheckName() const { return CheckName; }
  std::string CheckName;
}

but in this case (Replacement.h:141)

typedef std::set<Replacement> Replacements;

should be changed to

typedef std::set<ExtendedReplacement> Replacements;

which means a lot of code will be changed.

Is that an acceptable change? Or you meant another kind of wrapper?

What kind of wrapper should it be?
I was thinking of this kind:

class ExtendedReplacement : public Replacement {

No, I meant a different thing: serialize ClangTidyError or make a specialized structure for this purpose. Apart from replacements it contains the diagnostic message, diagnostic location, and the check name. We'll just need to teach clang-apply-replacements to read this structure.

Now the class that is serialized is Diagnostics.
I've moved ClangTidyError and ClangTidyMessage to the upper level, and renamed to Diagnostics and DiagnosticsMessage. Now any tool can use this classes, they contain more information than than Replacement. I think Diagnostics is a good class to add some more information later.

Please, review the fix, if it's ok I'll run clang-format on the changed code and provide the documentation. Thanks!

Fixed YAML format (was not correct in the last patch).
Grouped replacements in YAML by Diagnostics. It will help to apply replacements for one fix at once.

Elijah_Th added a comment.EditedFeb 9 2016, 1:02 AM

YAML report looks like this now:

---
MainSourceFile:  ''
Diagnostics:     
  CheckName:       misc-macro-parentheses
  Replacements:    
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1354
      Length:          0
      ReplacementText: '('
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1355
      Length:          0
      ReplacementText: ')'
Diagnostics:     
  CheckName:       misc-macro-parentheses
  Replacements:    
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1452
      Length:          0
      ReplacementText: '('
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1453
      Length:          0
      ReplacementText: ')'
...
alexfh added a comment.Apr 1 2016, 9:09 PM

Sorry for the loooong delay. I missed you patch among all other ones. Could you rebase your patch on top of HEAD and clang-format the files you changed? I'll come back with more substantial comments early next week.

alexfh requested changes to this revision.Apr 7 2016, 11:28 AM
alexfh edited edge metadata.

The main concern here is that the clang-apply-replacements tool should be able to read this format. See the clang/tools/extra/clang-tidy/tool/run-clang-tidy.py script for an example of its usage.

This revision now requires changes to proceed.Apr 7 2016, 11:28 AM

alexfh,

OK. I'll take a look at apply-replacements and fix if needed, in the beginning of the next week!

alexfh added a comment.Mar 1 2017, 2:31 AM

This patch is superseded by D26137.

alexfh commandeered this revision.Mar 1 2017, 2:32 AM
alexfh edited reviewers, added: Elijah_Th; removed: alexfh.
alexfh abandoned this revision.Mar 1 2017, 2:32 AM