Skip to content

Conversation

@ZijunZhaoCCK
Copy link
Contributor

LONG_LONG_ macros are not GNU-only extensions any more. Bionic also defines them.

@ZijunZhaoCCK ZijunZhaoCCK added the clang Clang issues not falling into any other category label Nov 8, 2024
@llvmbot llvmbot added backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Nov 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: None (ZijunZhaoCCK)

Changes

LONG_LONG_ macros are not GNU-only extensions any more. Bionic also defines them.


Full diff: https://github.com/llvm/llvm-project/pull/115406.diff

1 Files Affected:

  • (modified) clang/lib/Headers/limits.h (+5-4)
diff --git a/clang/lib/Headers/limits.h b/clang/lib/Headers/limits.h
index 56dffe568486cc..9879fe81faa651 100644
--- a/clang/lib/Headers/limits.h
+++ b/clang/lib/Headers/limits.h
@@ -111,11 +111,12 @@
 #define ULLONG_MAX (__LONG_LONG_MAX__*2ULL+1ULL)
 #endif
 
-/* LONG_LONG_MIN/LONG_LONG_MAX/ULONG_LONG_MAX are a GNU extension.  It's too bad
-   that we don't have something like #pragma poison that could be used to
-   deprecate a macro - the code should just use LLONG_MAX and friends.
+/* LONG_LONG_MIN/LONG_LONG_MAX/ULONG_LONG_MAX are a GNU extension. Bionic also
+   defines them. It's too bad that we don't have something like #pragma poison
+   that could be used to deprecate a macro - the code should just use LLONG_MAX
+   and friends.
  */
-#if defined(__GNU_LIBRARY__) ? defined(__USE_GNU) : !defined(__STRICT_ANSI__)
+#if (defined(__GNU_LIBRARY__) ? defined(__USE_GNU) : !defined(__STRICT_ANSI__)) || defined(__BIONIC__)
 
 #undef   LONG_LONG_MIN
 #undef   LONG_LONG_MAX

@github-actions
Copy link

github-actions bot commented Nov 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@pirama-arumuga-nainar pirama-arumuga-nainar left a comment

Choose a reason for hiding this comment

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

LGTM for Android; tagging @AaronBallman as well.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Release note and test coverage?

@ZijunZhaoCCK
Copy link
Contributor Author

Release note and test coverage?

I tested it locally. I copied code to prebuilt/.../limits.h in aosp and removed bionic's copy and rebuilt to see whether it works. And it worked well. Any extra upstream test coverage needed?

@AaronBallman
Copy link
Collaborator

Release note and test coverage?

I tested it locally. I copied code to prebuilt/.../limits.h in aosp and removed bionic's copy and rebuilt to see whether it works. And it worked well. Any extra upstream test coverage needed?

Yes, we've got tests for Clang-supplied headers in clang/test/Headers; you should pass -ffreestanding on the RUN line and that should find Clang's headers rather than the system headers.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM aside from some minor changes to the release note.

@ZijunZhaoCCK ZijunZhaoCCK merged commit 7d20ea9 into llvm:main Nov 14, 2024
7 of 8 checks passed
@ZijunZhaoCCK ZijunZhaoCCK deleted the feature/extend-macros branch November 14, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants