This is an archive of the discontinued LLVM Phabricator instance.

Added `applyAtomicChanges` function.
ClosedPublic

Authored by ioeric on Mar 9 2017, 3:17 AM.

Event Timeline

ioeric created this revision.Mar 9 2017, 3:17 AM
klimek added inline comments.Mar 9 2017, 3:33 AM
include/clang/Tooling/Refactoring/AtomicChange.h
129

That part of the sentence doesn't parse.

132

I'd add default values for everything that will otherwise be uninitialized.

147–149

Why?

lib/Tooling/Refactoring/AtomicChange.cpp
106

Won't this be incorrect if the last line (no \n) is 1 over the column limit (due to EndPos = Code.size()-1 above)?

ioeric updated this revision to Diff 91163.Mar 9 2017, 4:57 AM
ioeric marked 2 inline comments as done.
  • Addressed comments; stylized code.
ioeric added inline comments.Mar 9 2017, 4:59 AM
include/clang/Tooling/Refactoring/AtomicChange.h
147–149

Changes might have equivalent paths in different forms (e.g. relative or absolute), and checking canonical paths are dependent on file systems and the working directory, so I think this should be checked/handled in the user level.

lib/Tooling/Refactoring/AtomicChange.cpp
106

Yeah, that is a bug. Should be Code.size() then.

djasper added inline comments.Mar 9 2017, 11:20 AM
include/clang/Tooling/Refactoring/AtomicChange.h
139

Should probably be "exceeding the column limit".

lib/Tooling/Refactoring/AtomicChange.cpp
91

I think LLVM style is lowerCamelCase now, right? Here and below.

113

nit: I'd move this down and just return {} for kNone.

127

Remove braces?

151

What is copying these vectors buying us?

164

I'd remove the parentheses.

196

Remove the braces of the for loops.

ioeric updated this revision to Diff 91288.Mar 10 2017, 1:43 AM
ioeric marked 7 inline comments as done.
  • Addressed review comments.
djasper accepted this revision.Mar 22 2017, 11:54 AM

Looks good.. Very nice :)

lib/Tooling/Refactoring/AtomicChange.cpp
102

nit: remove braces (at least for consistency)

This revision is now accepted and ready to land.Mar 22 2017, 11:54 AM
ioeric updated this revision to Diff 93231.Mar 28 2017, 6:10 AM
  • addressed comment.
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
cfe/trunk/include/clang/Tooling/Refactoring/AtomicChange.h
19 ↗(On Diff #93232)

It requires clangFormat in LINK_LIBS. Fixed in r298921.

ioeric reopened this revision.Apr 7 2017, 2:37 AM

This (rL298913) was reverted upstream due to cyclic dependency on GreenDragon bot. Investigating...

This revision is now accepted and ready to land.Apr 7 2017, 2:37 AM
ioeric updated this revision to Diff 94497.Apr 7 2017, 2:37 AM
  • Try to unbreak buildbots after r298913.
  • Add clangFormat to dependency
  • Update cmake
ioeric updated this revision to Diff 108881.Jul 31 2017, 1:47 AM
  • Merged with origin/master
This revision was automatically updated to reflect the committed changes.