[PATCH] D44558: Change DT_* value definitions to macros in a separate file

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

This looks OK to me. Let's see what other reviewers think, my comments below.



================
Comment at: include/llvm/BinaryFormat/DynamicTags.def:1
+
+#ifndef DYNAMIC_TAG
----------------
Excessive empty line.


================
Comment at: include/llvm/BinaryFormat/DynamicTags.def:8
+// such as DT_HIOS, etc. to allow using this file to construct the
+// stringification switch statement for llvm-readobj
+
----------------
I am not sure it is worth to mention the particular tool name here, as such file can be used for very different purposes I think.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1548
     switch (Type) {
-    LLVM_READOBJ_TYPE_CASE(HEXAGON_SYMSZ);
-    LLVM_READOBJ_TYPE_CASE(HEXAGON_VER);
-    LLVM_READOBJ_TYPE_CASE(HEXAGON_PLT);
+#define HEXAGON_DYNAMIC_TAG(name, value)                                       \
+  case DT_##name:                                                              \
----------------
Personally  to me one-line short form here and below looks easier to read:

```
#define HEXAGON_DYNAMIC_TAG(name, value) case DT_##name: return #name;
```


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1569
+#define DYNAMIC_TAG_MARKER(name, value)
+#undef DYNAMIC_TAG
+#define DYNAMIC_TAG(name, value)                                               \
----------------
This undef should be right after the end of the first switch I think.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1582
 
 #undef LLVM_READOBJ_TYPE_CASE
 
----------------
This looks to be dead now.


Repository:
  rL LLVM

https://reviews.llvm.org/D44558