search

google / syzygy

Syzygy Transformation Toolchain
Apache License 2.0
355 stars 59 forks source link

Crash reporting policy code needs refactoring #45

Open sigurasg opened 8 years ago

sigurasg commented 8 years ago

In https://codereview.chromium.org/1992773002/diff/20001/syzygy/agent/asan/runtime.cc I failed to understand the crash reporting mode selection code in review. This wants refactoring to separate policy and mechanism to where the logic is human-readable.

sebmarchand commented 8 years ago

Hum, I'm not really familiar with this code, IIRC Chris own most of it, did you had a design in mind ?

sigurasg commented 8 years ago

This is further to Chris' comment on < https://codereview.chromium.org/1992773002/#msg10>:

Maybe it makes more sense to consolidate the policy decision somewhere, and to report the crash reporter used as the group?

I agree, it's scattered and messy. I really don't want to have to refactor this, and would much rather Seb own that.

On Fri, May 20, 2016 at 10:04 AM Sébastien Marchand < notifications@github.com> wrote:

Hum, I'm not really familiar with this code, IIRC Chris own most of it, did you had a design in mind ?

— You are receiving this because you were assigned. Reply to this email directly or view it on GitHub https://github.com/google/syzygy/issues/45#issuecomment-220614280

chhamilton commented 8 years ago

It's imposed legacy from the way the feature flags originally worked. I've ended up refactoring it a few times in an attempt to make it readable, but I think we're hitting a wall now. Since you were already refactoring the rest of the feature flag code I was hoping you could own pushing this all the way through.

sebmarchand commented 8 years ago

Sure, I'll look at this.