This is an archive of the discontinued LLVM Phabricator instance.

[pstl] Replace direct use of assert() with __PSTL_ASSERT
ClosedPublic

Authored by ldionne on Apr 3 2019, 10:38 PM.

Details

Summary

Standard libraries may (libstdc++ in particular) forbid direct use of
assert()/<cassert> in library code.

Event Timeline

rodgert created this revision.Apr 3 2019, 10:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 10:38 PM
ldionne added inline comments.Apr 5 2019, 1:22 PM
pstl/include/pstl/internal/pstl_config.h
17–28

I would do this:

#if !defined(_PSTL_ASSERT)
#  include <cassert>
#  define _PSTL_ASSERT(x) assert(x)
#endif

And I would just use _PSTL_ASSERT(x) elsewhere in the code. I don't see a reason to have a separate _PSTL_USE_ASSERT macro if we can define _PSTL_ASSERT directly. Also, whether NDEBUG is defined will control whether _PSTL_ASSERT does something by default, so the default is fine for unit tests. WDYT?

rodgert marked an inline comment as done.Apr 5 2019, 1:27 PM
rodgert added inline comments.
pstl/include/pstl/internal/pstl_config.h
17–28

That works for me. I will be injecting the definitions for PSTL_ASSERT() and PSTL_ASSERT_MSG() from libstdc++'s configuration anyway.

This is really just to try and provide a sane default the standalone impl. I'll update the diff.

rodgert updated this revision to Diff 194544.Apr 10 2019, 10:25 AM
  • [pstl] Supply definition of __PSTL_ASSERT() only if not already defined
ldionne requested changes to this revision.Apr 10 2019, 10:27 AM

LGTM, except for the nit.

pstl/test/CMakeLists.txt
31

Not needed anymore.

This revision now requires changes to proceed.Apr 10 2019, 10:27 AM
ldionne commandeered this revision.Nov 2 2020, 3:07 PM
ldionne edited reviewers, added: rodgert; removed: ldionne.
This revision now requires review to proceed.Nov 2 2020, 3:07 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 2 2020, 3:07 PM
Herald added a subscriber: jkorous. · View Herald Transcript
ldionne accepted this revision.Nov 2 2020, 3:36 PM
This revision is now accepted and ready to land.Nov 2 2020, 3:36 PM
ldionne updated this revision to Diff 302421.Nov 2 2020, 3:36 PM

Rebased onto master

ldionne accepted this revision.Nov 2 2020, 3:36 PM
This revision was landed with ongoing or failed builds.Nov 2 2020, 3:37 PM
This revision was automatically updated to reflect the committed changes.

I’ll make sure both sides agree on the name. I still need to go through the
tests on the libstdc++ side but that can wait until after stage1 closes.