This is an archive of the discontinued LLVM Phabricator instance.

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

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

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–154

s/Fixes/Replacements/

clang-tidy/ClangTidyDiagnosticConsumer.cpp
80–81

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
2

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

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

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–520

Please add braces for the outer loop.

clang-tidy/ClangTidyDiagnosticConsumer.cpp
81

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–500

Please add braces around the outer loop.

506–519

Please add braces for both outer loops.

unittests/clang-tidy/ClangTidyTest.h
122–129

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.