This is an archive of the discontinued LLVM Phabricator instance.

[DWARFv5] Turn an assert into a diagnostic.
ClosedPublic

Authored by probinson on Feb 9 2018, 6:29 PM.

Details

Summary

Hand-coded assembler files should not trigger assertions.

I'm not really keen on my choice of error indicators, which is why I'm
posting this for review; suggestions welcome. The thing is, there's a
moderate amount of plumbing in between the AsmParser and the DWARF
file table management that would be affected, that is also used by
DwarfDebug, and changing it would have largely no benefit.
I contemplated ErrorOr<> for a while, but it's not that easy to use
for a non-syscall diagnostic.

Diff Detail

Event Timeline

probinson created this revision.Feb 9 2018, 6:29 PM

Not sure what you mean by "a non-syscall diagnostic"? (there's StringError if you want to just put a string rather than an error code)

What is it that's undesirable about the error handling you've implemented here? Given that there's a similar error path nearby, that seems like a reasonable (or at least consistent) approach... but I've not looked at this area of the code in detail to see if there's something particularly undesirable about it.

Not sure what you mean by "a non-syscall diagnostic"? (there's StringError if you want to just put a string rather than an error code)

I mean, a case where a std::error_code is not what we want.

What is it that's undesirable about the error handling you've implemented here? Given that there's a similar error path nearby, that seems like a reasonable (or at least consistent) approach... but I've not looked at this area of the code in detail to see if there's something particularly undesirable about it.

Overloading two return values to indicate an error, instead of a more explicit indication. And then relying on callers to remember to check for them, instead of forcing callers to check for them.

probinson updated this revision to Diff 134774.Feb 16 2018, 5:03 PM

Use Expected<unsigned> instead of just unsigned to return errors.

aprantl accepted this revision.Feb 17 2018, 5:28 PM
This revision is now accepted and ready to land.Feb 17 2018, 5:28 PM
dblaikie accepted this revision.Feb 20 2018, 8:46 AM

Given the abundance of callers that know the call won't fail, perhaps it'd be appropriate to keep the original function name without Expected, then have a version of the function ('tryBlah') that returns Expected (& implement blah in terms of cantFail(tryBlah))

Given the abundance of callers that know the call won't fail, perhaps it'd be appropriate to keep the original function name without Expected, then have a version of the function ('tryBlah') that returns Expected (& implement blah in terms of cantFail(tryBlah))

That would certainly be less churn, and I'm happy to go that way.

This revision was automatically updated to reflect the committed changes.