This is an archive of the discontinued LLVM Phabricator instance.

Raise compile error when using unimplemented functions
ClosedPublic

Authored by muiez on Oct 15 2021, 8:58 AM.

Details

Summary

The path functions in this patch are unimplemented (as per the TODO comment from upstream). To avoid running into a linker error (missing symbol), this patch raises a compile error by commenting out the functions, which is more user friendly.

Diff Detail

Event Timeline

muiez requested review of this revision.Oct 15 2021, 8:58 AM
muiez created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2021, 8:58 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

Above my pay grade, but...

(1) Isn't a runtime error vastly less user-friendly than a link-time error? Perhaps you could show an example of the kind of program that would run into this linker error and/or assertion?
(2) Is a plain assert really more appropriate than either _LIBCPP_ASSERT or throw? (I think _LIBCPP_ASSERT would be less appropriate because it compiles away in non-debug modes; but then, so does regular assert in NDEBUG mode.)
(3) Alternatively, how about either just commenting-out, or =delete'ing, these functions, to turn the linker error into a compile-time (and SFINAE'able) ill-formedness?

muiez updated this revision to Diff 380089.Oct 15 2021, 1:08 PM

Cause compile time error instead.

muiez added a comment.Oct 15 2021, 1:09 PM

Above my pay grade, but...

(1) Isn't a runtime error vastly less user-friendly than a link-time error? Perhaps you could show an example of the kind of program that would run into this linker error and/or assertion?
(2) Is a plain assert really more appropriate than either _LIBCPP_ASSERT or throw? (I think _LIBCPP_ASSERT would be less appropriate because it compiles away in non-debug modes; but then, so does regular assert in NDEBUG mode.)
(3) Alternatively, how about either just commenting-out, or =delete'ing, these functions, to turn the linker error into a compile-time (and SFINAE'able) ill-formedness?

I updated the revision to cause a compile time error instead.

Mordante accepted this revision as: Mordante.Oct 16 2021, 6:29 AM
Mordante added a subscriber: Mordante.

This patch seems to be a generic libc++ patch and not specific to z/OS right? If so please update the title of the path.
Please update the title and description of patch to describe the implementation using = delete.
Aside from that LGTM.

muiez retitled this revision from [SystemZ][z/OS] Raise assertion error when using unimplemented functions to Raise assertion error when using unimplemented functions.Oct 18 2021, 7:11 AM
muiez edited the summary of this revision. (Show Details)
ldionne accepted this revision.Oct 18 2021, 8:14 AM

I agree it's weird we have those declarations but no implementation for them. I think my preferred approach here would be to just comment out those functions. That way, we know for sure we won't interact in weird ways with SFINAE or anything.

Commenting out the functions is what's closest to just not having implementing them (which is what's going on here).

Apart from that, LGTM.

This revision is now accepted and ready to land.Oct 18 2021, 8:14 AM
muiez updated this revision to Diff 380744.Oct 19 2021, 11:28 AM
muiez retitled this revision from Raise assertion error when using unimplemented functions to Raise compile error when using unimplemented functions.
muiez edited the summary of this revision. (Show Details)

Commented out unimplemented functions.

This revision was automatically updated to reflect the committed changes.