[PATCH] D33552: [ELF] - Make implementation of .gdb index to be more natural for futher paralleling.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall, this patch is a bit too scarce of comments. You want to keep it readable for those who don't know what the code is intended to do.



================
Comment at: ELF/GdbIndex.h:31-32
+struct CompilationUnitEntry {
+  size_t CuOffset;
+  size_t CuLength;
+};
----------------
uint64_t?


================
Comment at: ELF/GdbIndex.h:40
+
+struct FileGdbIndexData {
+  std::vector<AddressEntry> AddressArea;
----------------
There seems a better name than `FileGdbIndexData`.  This represent a chunk of data you read from a DWARF section, right?


================
Comment at: ELF/SyntheticSections.cpp:1752
 
-void GdbIndexSection::readDwarf(InputSection *Sec) {
+static std::vector<InputSection *> getInfoSections() {
+  std::vector<InputSection *> Ret;
----------------
getInfo -> getDebugInfo


================
Comment at: ELF/SyntheticSections.cpp:1766-1768
+  IndexData.resize(V.size());
+  for (size_t I = 0; I < V.size(); ++I)
+    readDwarf(V[I], IndexData[I]);
----------------
Please write in a more natural way.

  for (InputSection *Sec : V)
    InputData.push_back(readDwarf(Sec));


================
Comment at: ELF/SyntheticSections.cpp:1788-1789
+
+      CuVectors[Sym->CuVectorIndex].push_back((uint32_t)CuId |
+                                              (NameType.Type << 24));
+    }
----------------
When you bitwise-or some values, you usually want to write values from MSB to LSB.


================
Comment at: ELF/SyntheticSections.cpp:1796
+
+void GdbIndexSection::readDwarf(InputSection *Sec, FileGdbIndexData& Data) {
   Expected<std::unique_ptr<object::ObjectFile>> Obj =
----------------
This should return FileGdbIndexData. (In general, you want to avoid mutating a object passed as a reference if writing in a more functional way is doable.)


================
Comment at: ELF/SyntheticSections.cpp:1812
 
-    CuVectors[Sym->CuVectorIndex].push_back({CuId, Pair.second});
-  }
+static size_t findCUSize(std::vector<FileGdbIndexData> &C) {
+  size_t Ret = 0;
----------------
This does not "find" anything. It computes. get, compute or calculate are good choices.


================
Comment at: ELF/SyntheticSections.cpp:1819
+
+static size_t findAddrAreaSize(std::vector<FileGdbIndexData> &C) {
+  size_t Ret = 0;
----------------
Addr -> Address


================
Comment at: ELF/SyntheticSections.h:514
   GdbHashTab SymbolTable;
-
-  // The CU vector portion of the constant pool.
-  std::vector<std::vector<std::pair<uint32_t, uint8_t>>> CuVectors;
-
-  std::vector<AddressEntry> AddressArea;
+  std::vector<FileGdbIndexData> IndexData;
 
----------------
`IndexData` is not a good variable name.


https://reviews.llvm.org/D33552