Page MenuHomePhabricator

[asan] Reify ErrorDeadlySignal
ClosedPublic

Authored by filcab on Aug 25 2016, 7:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 69244.Aug 25 2016, 7:37 AM
filcab retitled this revision from to Reify ErrorDeadlySignal.
filcab updated this object.
filcab added reviewers: kcc, samsonov.
filcab added a subscriber: llvm-commits.
filcab updated this revision to Diff 69809.Aug 31 2016, 1:26 AM

Updating with current tree.

filcab edited reviewers, added: eugenis, vitalybuka; removed: samsonov.Aug 31 2016, 1:32 AM
vitalybuka requested changes to this revision.Sep 1 2016, 12:40 PM
vitalybuka edited edge metadata.
vitalybuka added inline comments.
lib/asan/asan_errors.h
23 ↗(On Diff #69809)

Please move it back into cc file

lib/asan/asan_report.cc
324 ↗(On Diff #69809)

can you avoid nolint? maybe = {}

This revision now requires changes to proceed.Sep 1 2016, 12:40 PM
filcab added a comment.Sep 5 2016, 9:05 AM

As for the NOLINT on the constructor: We have two Error* constructors doing that.
Do you want me to change them to parenthesized initialization too, so we don't need the NOLINT?

lib/asan/asan_errors.h
23 ↗(On Diff #69809)

I had taken it out because it's a simple function which would now be needed on two .cc files (for a while), and we don't really want to export the symbol.
Will put it back and export it, though.

lib/asan/asan_report.cc
324 ↗(On Diff #69809)

I can use parenthesis, but we might want to start thinking about updating the cpplint.py script. I guess there are newer versions that might be able to deal with this.

filcab updated this revision to Diff 70336.Sep 5 2016, 9:07 AM
filcab edited edge metadata.

Addressed code review

filcab retitled this revision from Reify ErrorDeadlySignal to [asan] Reify ErrorDeadlySignal.Sep 5 2016, 9:15 AM
filcab edited edge metadata.
vitalybuka requested changes to this revision.Sep 6 2016, 11:07 AM
vitalybuka edited edge metadata.
vitalybuka added inline comments.
lib/asan/asan_errors.h
23 ↗(On Diff #70336)

I see no problem with symbol
Also we already have PrintGlobalNameIfASCII and PrintGlobalLocation, so this function could sit next to them in asan_report.cc and h
Please move it there.

136 ↗(On Diff #70336)

Let's keep NOLINT here for consistency,

lib/asan/asan_report.cc
324–325 ↗(On Diff #70336)

I see, it's about ";" after "}"
Feel free to keep NOLINT here as this code does a lot of this already,

This revision now requires changes to proceed.Sep 6 2016, 11:07 AM
filcab updated this revision to Diff 70524.Sep 7 2016, 6:19 AM
filcab edited edge metadata.
filcab marked an inline comment as done.

Updated with review feedback.

filcab updated this revision to Diff 70531.Sep 7 2016, 7:13 AM
filcab edited edge metadata.

Add a missing header include

vitalybuka accepted this revision.Sep 7 2016, 10:02 AM
vitalybuka edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 7 2016, 10:02 AM
This revision was automatically updated to reflect the committed changes.