[libcxx-commits] [PATCH] D80690: [RISCV] Support libunwind for riscv32

[Mitchell Horne via Phabricator via libcxx-commits]

mhorne added a comment.

Hi, sorry for the delay in review. It would be helpful if you could go through the inline comments and mark them as "done" if they have been addressed. I see there are still a few comments that have been ignored.

This patch is tricky because there are no consistent indentation rules for preprocessor directives. In general, I think the indentation is only necessary for blocks where it increases readability (such as where `_LIBUNWIND_CURSOR_SIZE` is defined, or the block where `reg_t` is defined). Otherwise, it's best to follow the existing convention of the file.



================
Comment at: libunwind/include/__libunwind_config.h:130-144
+#   define _LIBUNWIND_TARGET_RISCV 1
+#   if defined(__riscv_flen)
+#     define RISCV_FLEN __riscv_flen
+#   else
+#     define RISCV_FLEN 0
+#   endif
+#   define _LIBUNWIND_CONTEXT_SIZE (32 * (__riscv_xlen + RISCV_FLEN) / 64)
----------------
Please look at this block again carefully. It should follow a rule of 1 space of indentation for each level of nesting.


================
Comment at: libunwind/src/Registers.hpp:3982
+# else
+  (void) regNum;
+  return false;
----------------
`(void)regNum;`


================
Comment at: libunwind/src/Registers.hpp:3727
+// This is just for supresing undeclared error of fp_t
+   typedef double fp_t;
+#endif
----------------
mhorne wrote:
> Only the preprocessor directives need indentation, not the typedef.
Again, please fix these.


================
Comment at: libunwind/src/assembly.h:41
+
+# if __riscv_xlen == 64
+# define ILOAD ld
----------------
Everything inside this `#if` should have an additional space of indentation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80690/new/

https://reviews.llvm.org/D80690