[PATCH] D122840: [llvm-readobj][nfc] Run clang-format on the directory's .cpp and .h files
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 31 22:31:58 PDT 2022
jhenderson added inline comments.
================
Comment at: llvm/tools/llvm-readobj/ARMEHABIPrinter.h:1
-//===--- ARMEHABIPrinter.h - ARM EHABI Unwind Information Printer ----------===//
+//===--- ARMEHABIPrinter.h - ARM EHABI Unwind Information Printer ---------===//
//
----------------
Perhaps while you're at it put the C++ marker here.
================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:365
+ LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_FILE_MACHINE_THUMB),
+ LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_FILE_MACHINE_WCEMIPSV2)};
----------------
Not that I'm suggesting you change what clang-format has done here, but I find it odd that the closing brace is on the same line as an entry, but the opening one isn't. Perhaps worth asking a clang-format person about,
================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1765-1772
- case COFF::IMAGE_REL_BASED_ABSOLUTE: return "ABSOLUTE";
- case COFF::IMAGE_REL_BASED_HIGH: return "HIGH";
- case COFF::IMAGE_REL_BASED_LOW: return "LOW";
- case COFF::IMAGE_REL_BASED_HIGHLOW: return "HIGHLOW";
- case COFF::IMAGE_REL_BASED_HIGHADJ: return "HIGHADJ";
- case COFF::IMAGE_REL_BASED_ARM_MOV32T: return "ARM_MOV32(T)";
- case COFF::IMAGE_REL_BASED_DIR64: return "DIR64";
----------------
There's an active discussion on discourse about whether this style should be allowed. I'm not sure we should change it at this time, although preventing clang-format doing anything to it seems wrong too.
https://discourse.llvm.org/t/enable-single-line-case-statements-in-llvm-clang-format-style/61062
I've asked there for thoughts.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:83
#define ENUM_ENT(enum, altName) \
- { #enum, altName, ELF::enum }
+ { \
+#enum, altName, ELF::enum \
----------------
Looks like clang-format doesn't agree with itself?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:988-990
- {"None", "none", ELF::ELFCLASSNONE},
- {"32-bit", "ELF32", ELF::ELFCLASS32},
- {"64-bit", "ELF64", ELF::ELFCLASS64},
----------------
Here and throughout these tables, I think the existing style is more readable, so possibly decorate them (though do remove the extra space in this column for example).
If this style isn't available as a clang-format option, please make a feature request. If it is, I'd start a Discourse thread on enabling it, citing this code as an example.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2088
#define LLVM_READOBJ_DT_FLAG_ENT(prefix, enum) \
- { #enum, prefix##_##enum }
+ { \
+#enum, prefix##_##enum \
----------------
Another linter complaint.
================
Comment at: llvm/tools/llvm-readobj/WasmDumper.cpp:25
#define ENUM_ENTRY(X) \
- { #X, wasm::WASM_SYMBOL_TYPE_##X }
+ { \
+#X, wasm::WASM_SYMBOL_TYPE_##X \
----------------
And more here.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:141
#define ECase(X) \
- { #X, XCOFF::X }
+ { \
+#X, XCOFF::X \
----------------
And more (multiple times in this file).
================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.h:47
+#define LLVM_READOBJ_ENUM_ENT(ns, enum) \
+ { \
+#enum, ns::enum \
----------------
Ditto.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122840/new/
https://reviews.llvm.org/D122840
More information about the llvm-commits
mailing list