Page MenuHomePhabricator

Make initializeOutputStream() return false on error and true on success.
ClosedPublic

Authored by thakis on Sep 15 2018, 4:37 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Sep 15 2018, 4:37 PM

I'd like to know what the LLVM position is here before accepting a style patch like this. I can think of some VIPs (Very Important Procedures) that use either style, but my impression is that true on error is more common, which is why I wrote this function (and a bunch of other functions in the itanium demangler) like this. If you and @zturner feel strongly about this then I don't really mind either way. If you're interested in this issue then I think the right thing to do would be to get the LLVM community to standardize on one or the other.

False on Error os more common and in fact there has been some effort in
llvm to standardize. Several support functions have been changed in the
past because false on success is so confusing. If the function returns an
int (status) or an error that’s a different story, but with a bool false on
error and true on success I think has been generally agreed upon to be what
we should be doing for new code. Not formally in any kind of mailing list
discussion, but it’s come up in several reviews (just that I’ve seen) and
functions have been changed or rewritten to return false on failure

My two cents in, false on error makes more sense to me as it's not the same as integer return codes where 0 traditionally indicates success, it makes sense for a verb named function to return false on failure and true on success, or alternatively use a different way of reporting the status back.

erik.pilkington accepted this revision.Sep 16 2018, 12:04 PM

Okay, if false on error is generally agreed on and used more frequently then LGTM!

This revision is now accepted and ready to land.Sep 16 2018, 12:04 PM

Not formally in any kind of mailing list discussion

I've started a thread here: http://lists.llvm.org/pipermail/llvm-dev/2018-September/126178.html

Unless there's strong disagreement there, I'm going to land this in a few days.

This revision was automatically updated to reflect the committed changes.