Page MenuHomePhabricator

[clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings
Needs ReviewPublic

Authored by AlexanderLanin on Jan 14 2020, 12:53 PM.

Details

Summary

The generated file is now small enough to not hinder any post processing.

Create a yaml file to store suggested fixes in, which can be applied with clang-apply-replacements.

So it's enough to store the fixes, not all the warnings.
Before this change the resulting file was simply too large as it contained all warnings, least of which had FIX-ITs.

This is trivially achievable with no measureable runtime overhead during run-clang-tidy runs, instead of post processing where even parsing the file once is extremely slow.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 12:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Important: I'm not a python expert. It works fine as far as I can tell. I would feel better if someone with more than a day python experience would say that it is fine.

Eugene.Zelenko retitled this revision from [clang-tools-extra] run-clang-tidy -export-fixes exports only fixes, not all warnings to [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings.Jan 14 2020, 4:34 PM
Eugene.Zelenko edited reviewers, added: hokein, aaron.ballman, JonasToth; removed: alexfh_.
kimgr added a subscriber: kimgr.Jan 22 2020, 11:18 PM

A small Python suggestion.

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
112

This double lookup is unnecessary, you can do for note in diag.get("Notes", []): to default to empty list if "Notes" is not in diag.

114

Same here (not sure if Replacements holds a list, but I think it might)

Review findings applied (no real measurable difference in speed, maybe 100ms) but it's indeed more readable.
Without this fix the export took 12.96 seconds, with this patch 11.07 seconds.
Difference is even bigger when there are less auto fixable findings (70% with FIX-IT in my example).

As there is no test suite available I run this without and with the change and manually compared the output.
All warnings without FIX-IT have disappeared from exported, all warnings with FIX-IT are unmodified.

On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides -export-fixes there should be an -export-warnings or just -yaml-export?

On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides -export-fixes there should be an -export-warnings or just -yaml-export?

I like the idea of introducing -export-warnings (to export warnings completely - with messages, notes, ranges and fixes) and changing -export-fixes to only export fixes.

JonasToth resigned from this revision.Sep 13 2020, 3:06 AM