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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

I'd also like to see a clean CI run.



================
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))
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125958