Page MenuHomePhabricator

Convert OptimizationRemarkEmitter old emit() calls to new closure parameterized emit() calls

Authored by vivekvpandya on Sep 26 2017, 11:09 AM.

Diff Detail


Event Timeline

vivekvpandya created this revision.Sep 26 2017, 11:09 AM

changes Clang-formated!

anemet added inline comments.Oct 3 2017, 6:05 PM
781 ↗(On Diff #117558)

Move this inside the lambda and then also no need for {}

269–291 ↗(On Diff #117558)

auto RemarkBuilder = [&]() {

return OptimizationRemark(DEBUG_TYPE, "StackProtectorAllocaOrArray",
            << "Stack protection applied to function "
            << ore::NV("Function", F)
            << " due to a call to alloca or use of a variable length array";


and then later just:


936–950 ↗(On Diff #117558)

Move the if inside the lambda?

529–543 ↗(On Diff #117558)

Move 'if' inside the lambda

386–393 ↗(On Diff #117558)

Move the 'using namespace' inside the lambda. You have more of this later.

857 ↗(On Diff #117558)

The caller of this function already uses allowExtraAnalysis so no need to use the closure syntax here.

1251–1270 ↗(On Diff #117558)

Do this whole thing in the lambda.

5752–5754 ↗(On Diff #117558)

Again the point of the lambda to only conditionally construct the remark so please either build the remark inside or don't use the lambda syntax.

anemet edited edge metadata.Oct 3 2017, 6:06 PM

Also remember to add llvm-commit/cfe-commit when you create the review.

vivekvpandya marked 8 inline comments as done.

Updated patch to address @anemet 's comment.

anemet added a comment.Oct 9 2017, 8:54 AM

One more idea for improvement and then this is ready to go.

935–945 ↗(On Diff #118165)

Can you actually factor this a bit further so that the common parts are done unconditionally. The reason why we use an insertion operator so that we can do such things.

532–543 ↗(On Diff #118165)

Same thing here. Just build the common object and conditionally insert the discriminator.

vivekvpandya marked 2 inline comments as done.


anemet accepted this revision.Oct 10 2017, 8:46 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 10 2017, 8:46 PM
This revision was automatically updated to reflect the committed changes.