Skip to content

[libunwind] Use consistent indentation in __libunwind_config.h #152861

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgorny
Copy link
Member

@mgorny mgorny commented Aug 9, 2025

Use consistent indentation patterns all across __libunwind_config.h. The huge number of branches of the preprocessor conditions make them really unreadable when some of them are not indented.

@mgorny mgorny requested a review from a team as a code owner August 9, 2025 15:21
@llvmbot
Copy link
Member

llvmbot commented Aug 9, 2025

@llvm/pr-subscribers-libunwind

Author: Michał Górny (mgorny)

Changes

Use consistent indentation patterns all across __libunwind_config.h. The huge number of branches of the preprocessor conditions make them really unreadable when some of them are not indented.


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

1 Files Affected:

  • (modified) libunwind/include/__libunwind_config.h (+22-24)
diff --git a/libunwind/include/__libunwind_config.h b/libunwind/include/__libunwind_config.h
index bb7fe4c83a3c1..bf29132a8ebc4 100644
--- a/libunwind/include/__libunwind_config.h
+++ b/libunwind/include/__libunwind_config.h
@@ -136,17 +136,16 @@
 #    error "Unsupported MIPS ABI and/or environment"
 #  endif
 #  define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_MIPS
-#elif defined(__sparc__) && defined(__arch64__)
-#define _LIBUNWIND_TARGET_SPARC64 1
-#define _LIBUNWIND_HIGHEST_DWARF_REGISTER                                      \
-  _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC64
-#define _LIBUNWIND_CONTEXT_SIZE 33
-#define _LIBUNWIND_CURSOR_SIZE 45
+# elif defined(__sparc__) && defined(__arch64__)
+#  define _LIBUNWIND_TARGET_SPARC64 1
+#  define _LIBUNWIND_HIGHEST_DWARF_REGISTER LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC64
+#  define _LIBUNWIND_CONTEXT_SIZE 33
+#  define _LIBUNWIND_CURSOR_SIZE 45
 # elif defined(__sparc__)
-  #define _LIBUNWIND_TARGET_SPARC 1
-  #define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC
-  #define _LIBUNWIND_CONTEXT_SIZE 16
-  #define _LIBUNWIND_CURSOR_SIZE 23
+#  define _LIBUNWIND_TARGET_SPARC 1
+#  define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC
+#  define _LIBUNWIND_CONTEXT_SIZE 16
+#  define _LIBUNWIND_CURSOR_SIZE 23
 # elif defined(__riscv)
 #  define _LIBUNWIND_TARGET_RISCV 1
 #  if defined(__riscv_flen)
@@ -162,7 +161,7 @@
 #  else
 #   error "Unsupported RISC-V ABI"
 #  endif
-# define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_RISCV
+#  define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_RISCV
 # elif defined(__ve__)
 #  define _LIBUNWIND_TARGET_VE 1
 #  define _LIBUNWIND_CONTEXT_SIZE 67
@@ -173,20 +172,19 @@
 #  define _LIBUNWIND_CONTEXT_SIZE 34
 #  define _LIBUNWIND_CURSOR_SIZE 46
 #  define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_S390X
-#elif defined(__loongarch__)
-#define _LIBUNWIND_TARGET_LOONGARCH 1
-#if __loongarch_grlen == 64
-#define _LIBUNWIND_CONTEXT_SIZE 65
-#define _LIBUNWIND_CURSOR_SIZE 77
-#else
-#error "Unsupported LoongArch ABI"
-#endif
-#define _LIBUNWIND_HIGHEST_DWARF_REGISTER                                      \
-  _LIBUNWIND_HIGHEST_DWARF_REGISTER_LOONGARCH
-#elif defined(__wasm__)
+# elif defined(__loongarch__)
+#  define _LIBUNWIND_TARGET_LOONGARCH 1
+#  if __loongarch_grlen == 64
+#   define _LIBUNWIND_CONTEXT_SIZE 65
+#   define _LIBUNWIND_CURSOR_SIZE 77
+#  else
+#   error "Unsupported LoongArch ABI"
+#  endif
+#  define _LIBUNWIND_HIGHEST_DWARF_REGISTER _LIBUNWIND_HIGHEST_DWARF_REGISTER_LOONGARCH
+# elif defined(__wasm__)
 // Unused
-#define _LIBUNWIND_CONTEXT_SIZE 0
-#define _LIBUNWIND_CURSOR_SIZE 0
+#  define _LIBUNWIND_CONTEXT_SIZE 0
+#  define _LIBUNWIND_CURSOR_SIZE 0
 # else
 #  error "Unsupported architecture."
 # endif

Copy link

github-actions bot commented Aug 9, 2025

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

Use consistent indentation patterns all across `__libunwind_config.h`.
The huge number of branches of the preprocessor conditions make them
really unreadable when some of them are not indented.

Signed-off-by: Michał Górny <[email protected]>
@mgorny mgorny force-pushed the libunwind-indent branch from 6eecf8b to f4c0717 Compare August 9, 2025 15:27
Comment on lines +14 to +15
// Disable clang-format as it makes the huge conditions unreadable.
// clang-format off
Copy link
Member

Choose a reason for hiding this comment

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

If the point is making indentations correct, why disable clang-format?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's removing all indentation completely.

Copy link
Member

@aheejin aheejin Aug 11, 2025

Choose a reason for hiding this comment

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

Sorry I thought this fixed incorrect indentations?

Copy link
Member Author

Choose a reason for hiding this comment

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

It just flattens everything.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry maybe I'm missing something, but your change doesn't look like it's flattening everything:
image
Also the PR description says "use consistent indentation", not "remove all indentation"

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, clang-format flattens everything, so I'm disabling it, so indentations remain meaningful.

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

Should we maybe just set IndentPPDirectives: AfterHash? We already do that in libc++, libc++abi and (I think) compiler-rt.

@mgorny
Copy link
Member Author

mgorny commented Aug 11, 2025

Should we maybe just set IndentPPDirectives: AfterHash? We already do that in libc++, libc++abi and (I think) compiler-rt.

I suppose it would make sense to me, but I don't feel like I'm the person to make that decision.

@aheejin
Copy link
Member

aheejin commented Aug 11, 2025

Should we maybe just set IndentPPDirectives: AfterHash? We already do that in libc++, libc++abi and (I think) compiler-rt.

Ah I didn't know we needed this to support preprocessor indentation. I thought it was by default on because I've seen header indentations in other libraries.

If this is the case I also think setting that would be better than disabling clang-format. I'm not a libunwind maintainer so I'm not sure if I can decide on that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants