Page MenuHomePhabricator

[SanitizerCommon] Test `CombinedAllocator::ForEachChunk()` in unit tests.
ClosedPublic

Authored by delcypher on Nov 23 2018, 6:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Nov 23 2018, 6:30 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptNov 23 2018, 6:30 AM
kubamracek accepted this revision.Nov 26 2018, 8:52 AM

LGTM, of course (more tests, yay!).

The indentation makes this hard to read though. Maybe declare the lambda as a separate variable?

This revision is now accepted and ready to land.Nov 26 2018, 8:52 AM

LGTM, of course (more tests, yay!).

The indentation makes this hard to read though. Maybe declare the lambda as a separate variable?

The indentation is clang-format's doing. I'm a bit hesitant to put the lambda in a variable because it's only used in one place so putting it inline is the more standard thing to do.
However, I don't have a super strong opinion here so if you think it's clearer then I'll make the change.

delcypher updated this revision to Diff 175504.Nov 27 2018, 9:13 AM

Make inline lambda into a variable instead for readability.

LGTM, of course (more tests, yay!).

The indentation makes this hard to read though. Maybe declare the lambda as a separate variable?

The indentation is clang-format's doing. I'm a bit hesitant to put the lambda in a variable because it's only used in one place so putting it inline is the more standard thing to do.
However, I don't have a super strong opinion here so if you think it's clearer then I'll make the change.

@kubamracek I tried assigning the lambda to a local variable. You're right, it is more readable. I've updated this patch with that change.

@kcc Is it okay to land this?

I'm just going to land this. It's a testing only change so it's not going to break anything that actually ships. I can revert it if necessary if there is a problem.

This revision was automatically updated to reflect the committed changes.