This is an archive of the discontinued LLVM Phabricator instance.

Differential D150942

[libc] Make predicate macros be usable in `if constexpr (...)` contexts.
Needs ReviewPublic

Authored by sivachandra on May 19 2023, 1:10 AM.

Details

Reviewers
gchatelet
Summary

DO NOT SUBMIT - sending it out just to collect opinion.

Diff Detail

Event Timeline

sivachandra created this revision.May 19 2023, 1:10 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 19 2023, 1:10 AM
Herald added subscribers: libc-commits, ecnelises, tschuett. · View Herald Transcript
sivachandra requested review of this revision.May 19 2023, 1:10 AM
sivachandra added inline comments.May 19 2023, 1:12 AM
libc/src/__support/macros/properties/architectures.h
44

This allows us to use LIBC_TARGET_ARCH_IS_ARM is all of the following contexts:

#ifdef LIBC_TARGET_ARCH_IS_ARM
 ...
#endif

#ifndef LIBC_TARGET_ARCH_IS_ARM
...
#endif

if constexpr (LIBC_TARGET_ARCH_IS_ARM) {
 ...
}
Harbormaster completed remote builds in B233106: Diff 523684.May 19 2023, 1:16 AM
gchatelet added a comment.May 22 2023, 2:19 AM

The pattern is smart but I can see two issues with it:

  1. Depending on the context, the literal (i.e. LIBC_TARGET_ARCH_IS_ARM) is a preprocessor definition or a variable, this can be confusing.
  2. It's easy to mess up when defining (or renaming) the preprocessor definitions.

Also, we can already achieve the same behavior - although I agree it is more verbose.

if constexpr (LLVM_LIBC_IS_DEFINED(LIBC_TARGET_ARCH_IS_ARM)) {
 ...
}

I'm not completely sure it's worth the complexity.

Revision Contents

PathSize
libc/
src/
__support/
macros/
properties/
5 lines

Diff 523684

libc/src/__support/macros/properties/architectures.h

Show All 32 Lines
#define LIBC_TARGET_ARCH_IS_X86_64 #define LIBC_TARGET_ARCH_IS_X86_64
#endif #endif
#if defined(LIBC_TARGET_ARCH_IS_X86_32) || defined(LIBC_TARGET_ARCH_IS_X86_64) #if defined(LIBC_TARGET_ARCH_IS_X86_32) || defined(LIBC_TARGET_ARCH_IS_X86_64)
#define LIBC_TARGET_ARCH_IS_X86 #define LIBC_TARGET_ARCH_IS_X86
#endif #endif
#if (defined(__arm__) || defined(_M_ARM)) #if (defined(__arm__) || defined(_M_ARM))
#define LIBC_TARGET_ARCH_IS_ARM #define LIBC_TARGET_ARCH_IS_ARM LIBC_TARGET_ARCH_IS_ARM
inline constexpr bool LIBC_TARGET_ARCH_IS_ARM = true;
#else
inline constexpr bool LIBC_TARGET_ARCH_IS_ARM = false;
sivachandraAuthorUnsubmitted
Done
ReplyInline Actions

This allows us to use LIBC_TARGET_ARCH_IS_ARM is all of the following contexts:

#ifdef LIBC_TARGET_ARCH_IS_ARM
 ...
#endif

#ifndef LIBC_TARGET_ARCH_IS_ARM
...
#endif

if constexpr (LIBC_TARGET_ARCH_IS_ARM) {
 ...
}
sivachandra: This allows us to use `LIBC_TARGET_ARCH_IS_ARM` is all of the following contexts: ``` #ifdef…
#endif #endif
#if defined(__aarch64__) || defined(__arm64__) || defined(_M_ARM64) #if defined(__aarch64__) || defined(__arm64__) || defined(_M_ARM64)
#define LIBC_TARGET_ARCH_IS_AARCH64 #define LIBC_TARGET_ARCH_IS_AARCH64
#endif #endif
#if defined(__riscv) && (__riscv_xlen == 64) #if defined(__riscv) && (__riscv_xlen == 64)
#define LIBC_TARGET_ARCH_IS_RISCV64 #define LIBC_TARGET_ARCH_IS_RISCV64
Show All 11 Lines