Page MenuHomePhabricator

Make tooling::applyAllReplacements return llvm::Expected<string> instead of empty string to indicate potential error.
ClosedPublic

Authored by ioeric on Jun 22 2016, 2:04 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric updated this revision to Diff 61518.Jun 22 2016, 2:04 AM
ioeric retitled this revision from to Make tooling::applyAllReplacements return llvm::Expected<string> instead of empty string to indicate potential error..
ioeric updated this object.
ioeric added reviewers: klimek, djasper.
ioeric added a subscriber: cfe-commits.
klimek added inline comments.Jun 23 2016, 5:24 AM
include/clang/Tooling/Core/Replacement.h
170 ↗(On Diff #61518)

When I read "result code" I imagine a number.
Perhaps "returns the code with the replacements applied"
or "resulting code"?

lib/Format/Format.cpp
1395 ↗(On Diff #61518)

Why would we want to return the same replacements?

ioeric added inline comments.Jun 23 2016, 7:07 AM
lib/Format/Format.cpp
1395 ↗(On Diff #61518)

this is just a temp solution to keep the current interfaces of processReplacements and formatReplacements...should I add a FIXME or can I change them in this patch as well?

klimek added inline comments.Jun 23 2016, 7:13 AM
lib/Format/Format.cpp
1395 ↗(On Diff #61518)

Both are good options :) Choose one.

ioeric updated this revision to Diff 61780.Jun 24 2016, 5:45 AM
  • fixed commenting.
  • Make formatReplacemnts and cleanupAroundReplacements return llvm::Exppected<...>.
ioeric marked 4 inline comments as done.Jun 24 2016, 5:47 AM

mark previous comments as done

klimek added inline comments.Jun 24 2016, 6:29 AM
include/clang/Format/Format.h
780 ↗(On Diff #61780)

typo: otheriwse

lib/Format/Format.cpp
1395–1396 ↗(On Diff #61780)

Comment doesn't apply any more, right?

unittests/Format/CleanupTest.cpp
258 ↗(On Diff #61780)

The explicit cast is unfortunate. Does this not have an .Ok() method or something?

ioeric marked 2 inline comments as done.Jun 24 2016, 6:37 AM
ioeric added inline comments.
unittests/Format/CleanupTest.cpp
258 ↗(On Diff #61780)

unfortunately, the bool operator is the only way we got...

I think the idea is that Errors are always expected to be checked and handled (the destructor of llvm::Error asserts that an Error has been handled).

ioeric updated this revision to Diff 61785.Jun 24 2016, 6:38 AM
  • Addressed comments.
klimek added inline comments.Jun 24 2016, 8:42 AM
unittests/Format/CleanupTest.cpp
258 ↗(On Diff #61785)

Hm. Can we make it work with EXPECT_TRUE somehow?

ioeric added a subscriber: ioeric.Jun 24 2016, 9:01 AM

Would EXPECTED_FALSE(!Code) be better?

klimek edited edge metadata.Jun 24 2016, 9:02 AM

Would EXPECTED_FALSE(!Code) be better?

Not really. Can we at least use static_cast<bool>(...)?

ioeric updated this revision to Diff 61823.Jun 24 2016, 1:20 PM
ioeric edited edge metadata.
  • Addressed reviewer's comments
ioeric marked 3 inline comments as done.Jun 28 2016, 1:53 AM

Friendly ping

klimek accepted this revision.Jul 4 2016, 6:40 AM
klimek edited edge metadata.

lg

This revision is now accepted and ready to land.Jul 4 2016, 6:40 AM