Page MenuHomePhabricator

Fix clang-tidy crash when a single fix is applied on multiple files.
ClosedPublic

Authored by ioeric on Aug 8 2016, 3:00 AM.

Details

Summary

tooling::Replacements only holds replacements for a single file, so
this patch makes Fix a map from file paths to tooling::Replacements so that it
can be applied on multiple files.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric updated this revision to Diff 67137.Aug 8 2016, 3:00 AM
ioeric retitled this revision from to Fix clang-tidy crash when a single fix is applied on multiple files..
ioeric updated this object.
ioeric added reviewers: alexfh, hokein.
ioeric added a subscriber: cfe-commits.
hokein added inline comments.Aug 8 2016, 3:44 AM
clang-tidy/ClangTidyDiagnosticConsumer.h
66 ↗(On Diff #67137)

Use llvm::StringMap here?

alexfh requested changes to this revision.Aug 8 2016, 7:04 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/ClangTidy.cpp
129 ↗(On Diff #67137)

s/Fixes/Replacements/

clang-tidy/ClangTidyDiagnosticConsumer.cpp
80 ↗(On Diff #67137)

nit: 1. replace is a verb; 2. variable names should start with a capital.

Replacement would be better.

test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp
1 ↗(On Diff #67137)

I think, cat x > y is preferred over cp x y in lit tests as a more platform-agnostic way of copying files.

This revision now requires changes to proceed.Aug 8 2016, 7:04 AM
ioeric updated this revision to Diff 67164.Aug 8 2016, 7:35 AM
ioeric edited edge metadata.
ioeric marked 4 inline comments as done.
  • Addressed reviewers' comments.
hokein added inline comments.Aug 8 2016, 8:21 AM
test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h
1 ↗(On Diff #67164)

Usually test should not use STL headers as it relies on the system headers.

You don't have to use std::string to reproduce the crash, IMO.

alexfh added inline comments.Aug 8 2016, 9:13 AM
test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h
1 ↗(On Diff #67164)

Yep, just a class with a user-defined move constructor should be fine.

// .h file:
struct S {
  S(S&&);
  S(const S&);
};
struct Foo {
  Foo(const S &s);
  S s;
};

// .cpp file:
#include "a.h"
Foo::Foo(const S &s) : s(s) {}
alexfh requested changes to this revision.Aug 8 2016, 9:14 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Aug 8 2016, 9:14 AM
ioeric updated this revision to Diff 67193.Aug 8 2016, 9:56 AM
ioeric edited edge metadata.
ioeric marked 2 inline comments as done.
  • Update test to not use <string>
alexfh accepted this revision.Aug 8 2016, 3:06 PM
alexfh edited edge metadata.

A bunch of nits. Otherwise looks good.

Thank you for the fix!

clang-tidy/ClangTidy.cpp
519 ↗(On Diff #67193)

Please add braces for the outer loop.

clang-tidy/ClangTidyDiagnosticConsumer.cpp
81 ↗(On Diff #67193)

I was kind of intrigued by what auto stands for here. I think, llvm::Error is a less mysterious way of storing an error here ;)

498 ↗(On Diff #67193)

Please add braces around the outer loop.

506 ↗(On Diff #67193)

Please add braces for both outer loops.

unittests/clang-tidy/ClangTidyTest.h
122 ↗(On Diff #67193)

Please add braces here as well.

This revision is now accepted and ready to land.Aug 8 2016, 3:06 PM
Prazek added a subscriber: Prazek.Aug 8 2016, 8:42 PM

I remember that it was pissing me off when I used clang-tidy first time. Thanks for fixing that!

ioeric updated this revision to Diff 67292.Aug 9 2016, 12:54 AM
ioeric marked 4 inline comments as done.
ioeric edited edge metadata.
  • Nits fixed
This revision was automatically updated to reflect the committed changes.