Jump to conversation
Unresolved conversations (4)
@michaelrj-google michaelrj-google Nov 12, 2024
I think you also need to add `-ffixed-point` here.
...cmake/modules/CheckCompilerFeatures.cmake
@michaelrj-google michaelrj-google Nov 12, 2024
is this file supposed to have more? Maybe an `ifdef` to check the proper values are being defined? Notably, if you don't pass `-ffixed-point` to clang this will compile successfully, since `stdfix-macros.h` checks that before defining anything else.
...check_padding_on_unsigned_fixed_point.cpp
@michaelrj-google michaelrj-google Nov 12, 2024
nit: looks like you added an extra space here
...cmake/modules/CheckCompilerFeatures.cmake
@lntue lntue Nov 7, 2024
Why do we need these extra entry points built with this flag? Are they supposed to be on or off by default? If it's only for testing with and without the `-Xclang=-fpadding-on-unsigned-fixed-point`, then using `FLAGS` in the main entry points is more apt.
libc/src/stdfix/CMakeLists.txt
duncpro PiJoules
Resolved conversations (9)
@lntue lntue Nov 7, 2024
also remove this in favor of `get_value_len()`?
Outdated
libc/src/__support/fixed_point/fx_rep.h
duncpro
@lntue lntue Nov 5, 2024
needs `LIBC_INLINE`
Outdated
libc/src/__support/fixed_point/fx_rep.h
duncpro
@PiJoules PiJoules Nov 4, 2024
I'm probably being picky here, but could you also add some test cases for boring trivial values like -0.5, -0.25, 10, 42, etc.
libc/test/src/stdfix/CountlsTest.h
PiJoules duncpro
@PiJoules PiJoules Nov 4, 2024
nit: Perhaps to reduce copying `VALUE_LEN` N times in each specialization, you could have a free function ``` template <typename T> constexpr int GetValueLen() { return T::INTEGRAL_LEN + T::FRACTIONAL_LEN; } ```
Outdated
libc/src/__support/fixed_point/fx_rep.h
duncpro
@lntue lntue Nov 3, 2024
Since you already clear the sign bit for negative numbers, maybe just `& TOTAL_MASK` would be more efficient (aka optimized to no-op) for signed types.
Outdated
libc/src/__support/fixed_point/fx_bits.h
duncpro lntue
@lntue lntue Nov 3, 2024
isn't this just take the complement of the bits? Does the operator `~` work for fixed point types?
Outdated
libc/src/__support/fixed_point/fx_bits.h
duncpro lntue
@lntue lntue Nov 3, 2024
we should refactor this to be available for the whole `FXBits` class, and also add `TOTAL_MASK`.
Outdated
libc/src/__support/fixed_point/fx_bits.h
duncpro lntue
@lntue lntue Nov 3, 2024
nit: no `{}` for this inner `if` statement.
Outdated
libc/src/__support/fixed_point/fx_bits.h
duncpro
@lntue lntue Nov 1, 2024
nit: you can let `countls` targets to share the loop with `round`
Outdated
libc/src/stdfix/CMakeLists.txt
duncpro