[libc-commits] [PATCH] D127046: [libc][math] fmod/fmodf implementation.

[Kirill Okhotnikov via Phabricator via libc-commits]

orex added inline comments.


================
Comment at: libc/src/__support/FPUtil/builtin_wrappers.h:52
+
+template <typename T, bool CorrectZero = true> static inline int clz(T val) {
+  int result = __internal::clz<T>(val);
----------------
lntue wrote:
> orex wrote:
> > lntue wrote:
> > > In your use case, extra checks for 0 input are not needed, so commenting on the undefined behavior of `builtin_clz` when inputs are 0 is good enough.  A safe variant with extra checks could be added, but it should use different name.  One of the reason is that now if someone one to use `builtin_clz` without extra checks, they will have to specify the input type `T`:
> > > ```
> > >    clz<T, false>(...)
> > > ```
> > > So when the type `T` is known, or inside a non-template function, users cannot use type deduction: `clz<uint32_t, false>( ... )`.
> > > So in summary, my preference is that:
> > > - `clz` should just be a simple wrap around type matching for `__builtin_clz*`.
> > > - undefined behavior for inputs 0 can/should be documented.
> > > - safe versions can be added, but use different name so that type deduction can be applied.
> > I have a little bit different opinion for this case. If somebody would like to simply use `clz/ctz` he can use it without any complication and in a safe way. But, if somebody know, that he can improve the performance, skipping the test, it looks like OK to have such complications. For example. I don't see much difference between, adding new function have syntax like this `ctz<decltype(i), false>(i)`. Moreover such syntax can help to avoid implicit type conversion. Another point to use such template is to pass check zero parameter above easily. For example, make_value function can looks like `make_value<bool CheckZero>(...`. Of course it can be done with `if consexpr`, but template will be more straightforward.
> > 
> So from the readability standpoint of both implementers and reviewers, I think `clz(x)` and `safe_clz(x)` or `clz(x)` and `unsafe_clz(x)' are better to understand than `clz(x)` and `clz<delctype(x), false>(x)`.
> I'm also not a big fan of using boolean in template parameter, unless the template name makes it very clear what `true` and `false` mean.  If it's only used in one or a few tests, that's fine.  But this is on a utility header that will be used everywhere.  If we really want to pass these as template parameters, it's better to wrap them in functors, like `function<SafeClz>` vs `function<true>`.  Hope it makes sense?
> 
> Also, by changing the default `clz(x)` in this patch, you would need to update all current usages of it like in `sqrt` to the default version.
OK. You won.)))) I've change all occurrence to unsafe_ctz/clz.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046