This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Introduce __debug_db_insert_c()
ClosedPublic

Authored by philnik on Jan 10 2022, 8:06 AM.

Details

Summary

There are a lot of

#if _LIBCPP_DEBUG_LEVEL == 2
    __get_db()->__insert_c(this);
#endif

This patch introduces __debug_db_insert_c() to put the #if in one central place.

Diff Detail

Event Timeline

philnik requested review of this revision.Jan 10 2022, 8:06 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2022, 8:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Jan 10 2022, 8:09 AM
libcxx/include/__debug
271

You also probably want to make it constexpr after C++11?

libcxx/include/list
872

All of these calls should be _VSTD::__debug_db_insert_c.

philnik updated this revision to Diff 398650.Jan 10 2022, 8:16 AM
philnik marked 2 inline comments as done.
  • Address comments
ldionne accepted this revision.Jan 10 2022, 8:25 AM
This revision is now accepted and ready to land.Jan 10 2022, 8:25 AM

@ldionne has already accepted, but FWIW, I would have said let's not introduce a new function-call layer of indirection for this, because that harms -O0 codegen (not that we really care about -O0 codegen). I would have suggested a macro, or just not doing this at all. Food for thought maybe?

@ldionne has already accepted, but FWIW, I would have said let's not introduce a new function-call layer of indirection for this, because that harms -O0 codegen (not that we really care about -O0 codegen). I would have suggested a macro, or just not doing this at all. Food for thought maybe?

IMO we should try not to use macros when we have an easy alternative. And as you said, codegen at -O0 is not something that has a lot of weigh in the balance. I certainly wouldn't want to start making regular decisions based on that, at least.

We could add _LIBCPP_ALWAYS_INLINE. That eliminates the function call even at -O0 (https://godbolt.org/z/PffnTM9P5). But I personally wouldn't really want to do that.

philnik updated this revision to Diff 398761.Jan 10 2022, 3:36 PM
  • Add <type_traits> include
  • Make __debug_db_insert_c a template to forward the type
libcxx/include/__debug
15

Is this for __libcpp_is_constant_evaluated? Might it make sense to move __libcpp_is_constant_evaluated into <__config>?

272

and perhaps _LIBCPP_ALWAYS_INLINE as we discussed? (I'm still neutral/ambivalent — seems like an easy but also maybe negligible win.)

philnik added inline comments.Jan 10 2022, 3:57 PM
libcxx/include/__debug
15

Yes. But I don't think it should be in <__config>. It's not really a config. Maybe in something like __type_traits/is_contant_evaulated.h?

272

I'd like to hear what @ldionne thinks about this. I'm not really sold on the idea unless we have some general guidelines when to use _LIBCPP_ALWAYS_INLINE or something similar.

ldionne added inline comments.Jan 11 2022, 9:21 AM
libcxx/include/__debug
15

I would definitely not move it to __config. If anything, I would put it in __type_traits/is_constant_evaluated.h as suggested.

272

I would avoid sprinkling _LIBCPP_ALWAYS_INLINE unless we have very good reasons to. It's just a bad habit to take. I am confident that the optimizer will see this and inline it when debugging is disabled. When debugging is enabled, it doesn't really matter as much.

This revision was automatically updated to reflect the committed changes.