Skip to content

[libc][NFC] Fix missing LIBC_INLINE + style #73659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 29, 2023
Merged

[libc][NFC] Fix missing LIBC_INLINE + style #73659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95a5c4b
[libc][NFC] Fix missing LIBC_INLINE + style
gchatelet Nov 28, 2023
2bca7f1
fix formatting
gchatelet Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 28 additions & 26 deletions libc/src/__support/CPP/limits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_CPP_LIMITS_H
#define LLVM_LIBC_SRC___SUPPORT_CPP_LIMITS_H

#include "src/__support/macros/attributes.h" // LIBC_INLINE

#include <limits.h>

namespace LIBC_NAMESPACE {
Expand All @@ -24,74 +26,74 @@ constexpr unsigned long long ULLONG_MAX = ~0ULL;

template <class T> class numeric_limits {
public:
static constexpr T max();
static constexpr T min();
LIBC_INLINE static constexpr T max();
LIBC_INLINE static constexpr T min();
};

// TODO: Add numeric_limits specializations as needed for new types.

template <> class numeric_limits<int> {
public:
static constexpr int max() { return INT_MAX; }
static constexpr int min() { return INT_MIN; }
LIBC_INLINE static constexpr int max() { return INT_MAX; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already implied by the fact that the function is defined in the class declaration. @frobtech can we fix the clang -tidy to ignore this ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also applies to pretty much all code in these files)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fundamental misunderstanding of the purpose of LIBC_INLINE. It is never implicit, because it exists to be (sometimes) defined to something other than simply inline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read the documentation (libc/docs/code_style.rst), but it conflates LIBC_INLINE and inline:

Instead of using the ``inline`` keyword, they should be tagged with the   ``LIBC_INLINE``

(though it does mention implicitly inline functions):

 The ``LIBC_INLINE`` tag should also be added to functions which have
   definitions that are implicitly inline

I still kind of find the name misleading, maybe it should just avoid using inline which is already used for not-quite-the-same concept ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning is "where you'd use the inline keyword or it would be implied by the language spec", so I'm not at a loss for what different name would be more clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LIBC_INLINE macro implies inline, but some of our users (specifically Fuchsia) redefine the macro themselves to ensure that inlined functions also have additional properties. In Fuchsia's case they use it to enforce internal linkage. Linked below is their definition, along with a comment explaining why this is necessary.

https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/system/ulib/c/string/mem-aliases-include/src/__support/macros/attributes.h;l=24

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations. Can we put these in the doc (libc/docs/code_style.rst) ? There are probably going to be other people like me who are going to wonder about that, and those latter explanations make a lot of sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of today, the clang-tidy makes sure that LIBC_INLINE is in first position. This makes sense given Fuchsia' s definition but this makes it impossible to use LIBC_INLINE functions with attributes: e.g.:

[[nodiscard]] LIBC_INLINE constexpr int countl_zero(T value) ; // clang-tidy unhappy

LIBC_INLINE [[nodiscard]] constexpr int countl_zero(T value) ; // clang unhappy

LIBC_INLINE static constexpr int min() { return INT_MIN; }
};
template <> class numeric_limits<unsigned int> {
public:
static constexpr unsigned int max() { return UINT_MAX; }
static constexpr unsigned int min() { return 0; }
LIBC_INLINE static constexpr unsigned int max() { return UINT_MAX; }
LIBC_INLINE static constexpr unsigned int min() { return 0; }
};
template <> class numeric_limits<long> {
public:
static constexpr long max() { return LONG_MAX; }
static constexpr long min() { return LONG_MIN; }
LIBC_INLINE static constexpr long max() { return LONG_MAX; }
LIBC_INLINE static constexpr long min() { return LONG_MIN; }
};
template <> class numeric_limits<unsigned long> {
public:
static constexpr unsigned long max() { return ULONG_MAX; }
static constexpr unsigned long min() { return 0; }
LIBC_INLINE static constexpr unsigned long max() { return ULONG_MAX; }
LIBC_INLINE static constexpr unsigned long min() { return 0; }
};
template <> class numeric_limits<long long> {
public:
static constexpr long long max() { return LLONG_MAX; }
static constexpr long long min() { return LLONG_MIN; }
LIBC_INLINE static constexpr long long max() { return LLONG_MAX; }
LIBC_INLINE static constexpr long long min() { return LLONG_MIN; }
};
template <> class numeric_limits<unsigned long long> {
public:
static constexpr unsigned long long max() { return ULLONG_MAX; }
static constexpr unsigned long long min() { return 0; }
LIBC_INLINE static constexpr unsigned long long max() { return ULLONG_MAX; }
LIBC_INLINE static constexpr unsigned long long min() { return 0; }
};
template <> class numeric_limits<short> {
public:
static constexpr short max() { return SHRT_MAX; }
static constexpr short min() { return SHRT_MIN; }
LIBC_INLINE static constexpr short max() { return SHRT_MAX; }
LIBC_INLINE static constexpr short min() { return SHRT_MIN; }
};
template <> class numeric_limits<unsigned short> {
public:
static constexpr unsigned short max() { return USHRT_MAX; }
static constexpr unsigned short min() { return 0; }
LIBC_INLINE static constexpr unsigned short max() { return USHRT_MAX; }
LIBC_INLINE static constexpr unsigned short min() { return 0; }
};
template <> class numeric_limits<char> {
public:
static constexpr char max() { return CHAR_MAX; }
static constexpr char min() { return CHAR_MIN; }
LIBC_INLINE static constexpr char max() { return CHAR_MAX; }
LIBC_INLINE static constexpr char min() { return CHAR_MIN; }
};
template <> class numeric_limits<signed char> {
public:
static constexpr signed char max() { return SCHAR_MAX; }
static constexpr signed char min() { return SCHAR_MIN; }
LIBC_INLINE static constexpr signed char max() { return SCHAR_MAX; }
LIBC_INLINE static constexpr signed char min() { return SCHAR_MIN; }
};
template <> class numeric_limits<unsigned char> {
public:
static constexpr unsigned char max() { return UCHAR_MAX; }
static constexpr unsigned char min() { return 0; }
LIBC_INLINE static constexpr unsigned char max() { return UCHAR_MAX; }
LIBC_INLINE static constexpr unsigned char min() { return 0; }
};
#ifdef __SIZEOF_INT128__
// On platform where UInt128 resolves to __uint128_t, this specialization
// provides the limits of UInt128.
template <> class numeric_limits<__uint128_t> {
public:
static constexpr __uint128_t max() { return ~__uint128_t(0); }
static constexpr __uint128_t min() { return 0; }
LIBC_INLINE static constexpr __uint128_t max() { return ~__uint128_t(0); }
LIBC_INLINE static constexpr __uint128_t min() { return 0; }
};
#endif

Expand Down
6 changes: 4 additions & 2 deletions libc/src/__support/CPP/type_traits/invoke.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "src/__support/CPP/type_traits/is_pointer.h"
#include "src/__support/CPP/type_traits/is_same.h"
#include "src/__support/CPP/utility/forward.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE

namespace LIBC_NAMESPACE::cpp {

Expand All @@ -26,7 +27,7 @@ template <class FunctionPtrType> struct invoke_dispatcher {
template <class T, class... Args,
typename = cpp::enable_if_t<
cpp::is_same_v<cpp::decay_t<T>, FunctionPtrType>>>
static decltype(auto) call(T &&fun, Args &&...args) {
LIBC_INLINE static decltype(auto) call(T &&fun, Args &&...args) {
return cpp::forward<T>(fun)(cpp::forward<Args>(args)...);
}
};
Expand All @@ -37,7 +38,8 @@ struct invoke_dispatcher<FunctionReturnType Class::*> {
using FunctionPtrType = FunctionReturnType Class::*;

template <class T, class... Args, class DecayT = cpp::decay_t<T>>
static decltype(auto) call(FunctionPtrType fun, T &&t1, Args &&...args) {
LIBC_INLINE static decltype(auto) call(FunctionPtrType fun, T &&t1,
Args &&...args) {
if constexpr (cpp::is_base_of_v<Class, DecayT>) {
// T is a (possibly cv ref) type.
return (cpp::forward<T>(t1).*fun)(cpp::forward<Args>(args)...);
Expand Down
8 changes: 4 additions & 4 deletions libc/src/__support/CPP/utility/in_place.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,25 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_CPP_UTILITY_IN_PLACE_H
#define LLVM_LIBC_SRC___SUPPORT_CPP_UTILITY_IN_PLACE_H

#include "src/__support/macros/attributes.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE, LIBC_INLINE_VAR

#include <stddef.h> // size_t

namespace LIBC_NAMESPACE::cpp {

// in_place
struct in_place_t {
explicit in_place_t() = default;
LIBC_INLINE explicit in_place_t() = default;
};
LIBC_INLINE_VAR constexpr in_place_t in_place{};

template <class T> struct in_place_type_t {
explicit in_place_type_t() = default;
LIBC_INLINE explicit in_place_type_t() = default;
};
template <class T> LIBC_INLINE_VAR constexpr in_place_type_t<T> in_place_type{};

template <size_t I> struct in_place_index_t {
explicit in_place_index_t() = default;
LIBC_INLINE explicit in_place_index_t() = default;
};
template <size_t I>
LIBC_INLINE_VAR constexpr in_place_index_t<I> in_place_index{};
Expand Down
4 changes: 3 additions & 1 deletion libc/src/__support/CPP/utility/move.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@
#define LLVM_LIBC_SRC___SUPPORT_CPP_UTILITY_MOVE_H

#include "src/__support/CPP/type_traits/remove_reference.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE

namespace LIBC_NAMESPACE::cpp {

// move
template <class T> constexpr cpp::remove_reference_t<T> &&move(T &&t) {
template <class T>
LIBC_INLINE constexpr cpp::remove_reference_t<T> &&move(T &&t) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same: templates are implicitly inline

return static_cast<typename cpp::remove_reference_t<T> &&>(t);
}

Expand Down
8 changes: 5 additions & 3 deletions libc/src/__support/FPUtil/FPBits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "src/__support/CPP/type_traits.h"
#include "src/__support/bit.h"
#include "src/__support/common.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE

#include "FloatProperties.h"
#include <stdint.h>
Expand Down Expand Up @@ -106,13 +107,14 @@ template <typename T> struct FPBits {
// We don't want accidental type promotions/conversions, so we require exact
// type match.
template <typename XType, cpp::enable_if_t<cpp::is_same_v<T, XType>, int> = 0>
constexpr explicit FPBits(XType x) : bits(cpp::bit_cast<UIntType>(x)) {}
LIBC_INLINE constexpr explicit FPBits(XType x)
: bits(cpp::bit_cast<UIntType>(x)) {}

template <typename XType,
cpp::enable_if_t<cpp::is_same_v<XType, UIntType>, int> = 0>
constexpr explicit FPBits(XType x) : bits(x) {}
LIBC_INLINE constexpr explicit FPBits(XType x) : bits(x) {}

constexpr FPBits() : bits(0) {}
LIBC_INLINE constexpr FPBits() : bits(0) {}

LIBC_INLINE constexpr T get_val() const { return cpp::bit_cast<T>(bits); }

Expand Down
3 changes: 2 additions & 1 deletion libc/src/__support/FPUtil/FloatProperties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#define LLVM_LIBC_SRC___SUPPORT_FPUTIL_FLOATPROPERTIES_H

#include "src/__support/UInt128.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/properties/float.h" // LIBC_COMPILER_HAS_FLOAT128

#include <stdint.h>
Expand Down Expand Up @@ -139,7 +140,7 @@ template <> struct FPProperties<FPType::IEEE754_Binary128> {
};

//-----------------------------------------------------------------------------
template <typename FP> static constexpr FPType get_fp_type() {
template <typename FP> LIBC_INLINE static constexpr FPType get_fp_type() {
if constexpr (cpp::is_same_v<FP, float> && __FLT_MANT_DIG__ == 24)
return FPType::IEEE754_Binary32;
else if constexpr (cpp::is_same_v<FP, double> && __DBL_MANT_DIG__ == 53)
Expand Down
8 changes: 5 additions & 3 deletions libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "src/__support/CPP/bit.h"
#include "src/__support/UInt128.h"
#include "src/__support/common.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/properties/architectures.h"

#if !defined(LIBC_TARGET_ARCH_IS_X86)
Expand Down Expand Up @@ -103,19 +104,20 @@ template <> struct FPBits<long double> {
return bool((bits & FloatProp::SIGN_MASK) >> (FloatProp::BIT_WIDTH - 1));
}

constexpr FPBits() : bits(0) {}
LIBC_INLINE constexpr FPBits() : bits(0) {}

template <typename XType,
cpp::enable_if_t<cpp::is_same_v<long double, XType>, int> = 0>
constexpr explicit FPBits(XType x) : bits(cpp::bit_cast<UIntType>(x)) {
LIBC_INLINE constexpr explicit FPBits(XType x)
: bits(cpp::bit_cast<UIntType>(x)) {
// bits starts uninitialized, and setting it to a long double only
// overwrites the first 80 bits. This clears those upper bits.
bits = bits & ((UIntType(1) << 80) - 1);
}

template <typename XType,
cpp::enable_if_t<cpp::is_same_v<XType, UIntType>, int> = 0>
constexpr explicit FPBits(XType x) : bits(x) {}
LIBC_INLINE constexpr explicit FPBits(XType x) : bits(x) {}

LIBC_INLINE constexpr operator long double() {
return cpp::bit_cast<long double>(bits);
Expand Down
18 changes: 10 additions & 8 deletions libc/src/__support/UInt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "src/__support/CPP/type_traits.h"
#include "src/__support/bit.h" // unsafe_clz
#include "src/__support/integer_utils.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/optimization.h" // LIBC_UNLIKELY
#include "src/__support/math_extras.h" // SumCarry, DiffBorrow
#include "src/__support/number_pair.h"
Expand All @@ -28,13 +29,13 @@ template <size_t Bits, bool Signed> struct BigInt {

static_assert(Bits > 0 && Bits % 64 == 0,
"Number of bits in BigInt should be a multiple of 64.");
static LIBC_INLINE_VAR constexpr size_t WORDCOUNT = Bits / 64;
LIBC_INLINE_VAR static constexpr size_t WORDCOUNT = Bits / 64;
uint64_t val[WORDCOUNT]{};

static LIBC_INLINE_VAR constexpr uint64_t MASK32 = 0xFFFFFFFFu;
LIBC_INLINE_VAR static constexpr uint64_t MASK32 = 0xFFFFFFFFu;

static LIBC_INLINE constexpr uint64_t low(uint64_t v) { return v & MASK32; }
static LIBC_INLINE constexpr uint64_t high(uint64_t v) {
LIBC_INLINE static constexpr uint64_t low(uint64_t v) { return v & MASK32; }
LIBC_INLINE static constexpr uint64_t high(uint64_t v) {
return (v >> 32) & MASK32;
}

Expand Down Expand Up @@ -126,7 +127,8 @@ template <size_t Bits, bool Signed> struct BigInt {

LIBC_INLINE constexpr explicit operator bool() const { return !is_zero(); }

BigInt<Bits, Signed> &operator=(const BigInt<Bits, Signed> &other) = default;
LIBC_INLINE BigInt<Bits, Signed> &
operator=(const BigInt<Bits, Signed> &other) = default;

LIBC_INLINE constexpr bool is_zero() const {
for (size_t i = 0; i < WORDCOUNT; ++i) {
Expand Down Expand Up @@ -324,8 +326,8 @@ template <size_t Bits, bool Signed> struct BigInt {
// 196 3 9 6 2
// 256 4 16 10 3
// 512 8 64 36 7
constexpr BigInt<Bits, Signed>
LIBC_INLINE quick_mul_hi(const BigInt<Bits, Signed> &other) const {
LIBC_INLINE constexpr BigInt<Bits, Signed>
quick_mul_hi(const BigInt<Bits, Signed> &other) const {
BigInt<Bits, Signed> result(0);
BigInt<128, Signed> partial_sum(0);
uint64_t carry = 0;
Expand Down Expand Up @@ -893,7 +895,7 @@ template <> class numeric_limits<UInt<128>> {
LIBC_INLINE static constexpr UInt<128> max() {
return UInt<128>({0xffff'ffff'ffff'ffff, 0xffff'ffff'ffff'ffff});
}
static constexpr UInt<128> min() { return UInt<128>(0); }
LIBC_INLINE static constexpr UInt<128> min() { return UInt<128>(0); }
};

template <> class numeric_limits<Int<128>> {
Expand Down
6 changes: 4 additions & 2 deletions libc/src/__support/integer_operations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@
#define LLVM_LIBC_SRC___SUPPORT_INTEGER_OPERATIONS_H

#include "src/__support/CPP/type_traits.h"
#include "src/__support/macros/attributes.h" // LIBC_INLINE

namespace LIBC_NAMESPACE {

template <typename T>
static constexpr cpp::enable_if_t<cpp::is_integral_v<T>, T> integer_abs(T n) {
LIBC_INLINE static constexpr cpp::enable_if_t<cpp::is_integral_v<T>, T>
integer_abs(T n) {
return (n < 0) ? -n : n;
}

template <typename T>
static constexpr cpp::enable_if_t<cpp::is_integral_v<T>, void>
LIBC_INLINE static constexpr cpp::enable_if_t<cpp::is_integral_v<T>, void>
integer_rem_quo(T x, T y, T &quot, T &rem) {
quot = x / y;
rem = x % y;
Expand Down
4 changes: 2 additions & 2 deletions libc/src/string/memmem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(void *, memmem,
(const void *haystack, size_t haystack_len,
const void *needle, size_t needle_len)) {
constexpr auto comp = [](unsigned char l, unsigned char r) -> int {
constexpr auto COMP = [](unsigned char l, unsigned char r) -> int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change wanted ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although it could be done as a separate PR.

return l - r;
};
return inline_memmem(haystack, haystack_len, needle, needle_len, comp);
return inline_memmem(haystack, haystack_len, needle, needle_len, COMP);
}

} // namespace LIBC_NAMESPACE
6 changes: 6 additions & 0 deletions utils/bazel/llvm-project-overlay/libc/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ libc_support_library(
libc_support_library(
name = "__support_cpp_limits",
hdrs = ["src/__support/CPP/limits.h"],
deps = [
"__support_macros_attributes",
],
)

libc_support_library(
Expand Down Expand Up @@ -672,6 +675,7 @@ libc_support_library(
name = "__support_fputil_float_properties",
hdrs = ["src/__support/FPUtil/FloatProperties.h"],
deps = [
":__support_macros_attributes",
":__support_macros_properties_float",
":__support_uint128",
],
Expand All @@ -687,6 +691,7 @@ libc_support_library(
":__support_cpp_bit",
":__support_cpp_type_traits",
":__support_fputil_float_properties",
":__support_macros_attributes",
":__support_uint128",
],
)
Expand Down Expand Up @@ -746,6 +751,7 @@ libc_support_library(
":__support_fputil_fenv_impl",
":__support_fputil_fp_bits",
":__support_fputil_rounding_mode",
":__support_macros_attributes",
],
)

Expand Down