[libcxx-commits] [PATCH] D125958: [libc++] Use __libcpp_clz for a tighter __log2i function

[Hans Wennborg via Phabricator via libcxx-commits]

hans added inline comments.


================
Comment at: libcxx/include/__algorithm/sort.h:505
+  if (sizeof(__n) <= sizeof(unsigned))
+    return sizeof(unsigned) * __CHAR_BIT__ - 1 - __libcpp_clz(static_cast<unsigned>(__n));
+  if (sizeof(__n) <= sizeof(unsigned long))
----------------
philnik wrote:
> hans wrote:
> > philnik wrote:
> > > nilayvaish wrote:
> > > > I am wondering whether __CHAR_BIT__ is preferred over using CHAR_BIT (available from climit header file).  I don't know what the convention is for writing library code.
> > > I think `CHAR_BIT` is the preferred way. At least that's what I'm using.
> > It seems both are used roughly as often.
> > 
> > ```
> > $ git grep -In [^_]CHAR_BIT -- libcxx/include | wc -l
> > 25
> > $ git grep -In __CHAR_BIT__ -- libcxx/include | wc -l
> > 22
> > ```
> > 
> > Given that `__CHAR_BIT__` avoids potentially pushing another header include on every user of `<sort>`, I'd prefer to stay with it.
> I think that's manageable. Preprocessing `<climits>` produces 104 LOC while preprocessing `<algorithm>` produces 32888 LOC. That's not even a drop in the ocean. And climits is already included anyways. Using `CHAR_BIT` just makes the code a bit nicer to read. IMO we should actually replace the uses of `__CHAR_BIT__`. If you are worried about compile times you should use `-fmodules`.
I suppose if <climits> is already included, it doesn't matter in this instance. I'll leave it to someone else to decide whether `CHAR_BIT` or `__CHAR_BIT__` should be preferred in libc++ in general.

> If you are worried about compile times you should use -fmodules.

If only things were that simple :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125958/new/

https://reviews.llvm.org/D125958