Skip to content

Commit

Permalink
Fixed the compilation issue on macOS15 M platform
Browse files Browse the repository at this point in the history
  • Loading branch information
@monktan89
monktan89 committed Sep 27, 2024
1 parent f8add83 commit 9018fc9
Showing 1 changed file with 9 additions and 5 deletions.
14 changes: 9 additions & 5 deletions src/Util/util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,15 +583,19 @@ bool setThreadAffinity(int i) {
return false;
}

#if defined(__MACH__) || defined(__APPLE__)
#include <limits.h>
#include <mach-o/dyld.h> /* _NSGetExecutablePath */
#endif

#ifndef HAS_CXA_DEMANGLE
// We only support some compilers that support __cxa_demangle.
// TODO: Checks if Android NDK has fixed this issue or not.
#if defined(__ANDROID__) && (defined(__i386__) || defined(__x86_64__))
// TODO: Check if Android NDK has fixed this issue or not.
#if defined(ANDROID) && (defined(i386) || defined(x86_64))
#define HAS_CXA_DEMANGLE 0
#elif (__GNUC__ >= 4 || (__GNUC__ >= 3 && __GNUC_MINOR__ >= 4)) && \
!defined(__mips__)
#elif (GNUC >= 4 || (GNUC >= 3 && GNUC_MINOR >= 4)) && !defined(mips)
#define HAS_CXA_DEMANGLE 1
#elif defined(__clang__) && !defined(_MSC_VER)
#elif defined(clang) && !defined(_MSC_VER)
#define HAS_CXA_DEMANGLE 1
#else
#define HAS_CXA_DEMANGLE 0
Expand Down

1 comment on commit 9018fc9

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Util/util.cpp:

Code Review: Patch to src/Util/util.cpp

Summary

This patch addresses a compilation issue on macOS 15 M platform by modifying the HAS_CXA_DEMANGLE macro definition. It introduces platform-specific checks and updates the macro definition to ensure compatibility with different compilers and platforms.

Detailed Feedback

Code Overview

The patch modifies the HAS_CXA_DEMANGLE macro definition, which controls whether the __cxa_demangle function is used for demangling symbol names. The patch introduces platform-specific checks for macOS, Android, and other compilers to ensure correct macro definition based on the environment.

Strengths

  • Platform-Specific Checks: The patch introduces platform-specific checks for macOS, Android, and other compilers, which improves the accuracy and reliability of the HAS_CXA_DEMANGLE macro definition.
  • Compiler Compatibility: The patch updates the macro definition to ensure compatibility with different compilers, including GCC, Clang, and potentially others.

Areas for Improvement

1. Macro Naming and Consistency

  • Issue: The macro HAS_CXA_DEMANGLE is not consistently named across the code. In some places, it is used as HAS_CXA_DEMANGLE, while in others, it is used as HAS_CXA_DEMANGLE.
  • Suggestion: Ensure consistent naming throughout the code. Use either HAS_CXA_DEMANGLE or HAS_CXA_DEMANGLE consistently.
  • Example: 
    // Instead of:
#if HAS_CXA_DEMANGLE
// Use:
#if HAS_CXA_DEMANGLE

2. Documentation and Comments

  • Issue: The patch lacks clear documentation explaining the rationale behind the changes and the impact on different platforms.
  • Suggestion: Add comments explaining the purpose of the changes, the specific platforms affected, and the reasoning behind the updated macro definition.
  • Example: 
    // This patch introduces platform-specific checks for macOS, Android, and other compilers to ensure correct macro definition based on the environment.
// On macOS 15 M, the `__cxa_demangle` function is not available, so the macro is defined as 0.
// On Android, the `__cxa_demangle` function is not available in older NDK versions, so the macro is defined as 0.
// For other compilers, the macro is defined based on the compiler version and platform.
#if defined(__MACH__) || defined(__APPLE__)
#define HAS_CXA_DEMANGLE 0
#elif defined(ANDROID) && (defined(i386) || defined(x86_64))
#define HAS_CXA_DEMANGLE 0
// ...

3. Code Clarity and Readability

  • Issue: The code could be more readable and organized. The use of macros and conditional compilation can make it difficult to understand the logic.
  • Suggestion: Consider using more descriptive variable names, breaking down complex logic into smaller functions, and adding comments to improve code clarity.
  • Example: 
    // Instead of:
#if (GNUC >= 4 || (GNUC >= 3 && GNUC_MINOR >= 4)) && !defined(mips)
#define HAS_CXA_DEMANGLE 1
// Use:
bool isCxaDemangleSupported() {
    if (GNUC >= 4 || (GNUC >= 3 && GNUC_MINOR >= 4)) {
        if (!defined(mips)) {
            return true;
        }
    }
    return false;
}
#define HAS_CXA_DEMANGLE isCxaDemangleSupported()

Conclusion

The patch effectively addresses the compilation issue on macOS 15 M platform. However, it could be improved by addressing the issues related to macro naming, documentation, and code clarity. By incorporating these suggestions, the patch can be made more robust, maintainable, and easier to understand.

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.