[PATCH] D68062: Propeller lld framework for basicblock sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I haven't looked into the details yet, but here are some comments that are generally applicable to this patch.
After these are fixed, I may create some diffs for you to apply (they may be easier for me than adding comments here and there).



================
Comment at: lld/ELF/Propeller.cpp:1
+#include "Propeller.h"
+
----------------
Add license notice.


================
Comment at: lld/ELF/Propeller.cpp:125
+    }
+    if (L2.getAsInteger(16, SSize) == true) {
+      error("[Propeller]: Invalid size field, at propfile line: " +
----------------
Omit `== true`


================
Comment at: lld/ELF/Propeller.cpp:177
+
+  for (auto &S : BBSymbols) {
+    uint64_t SOrdinal;
----------------
Expand `auto` types unless they are obvious from the right hand side expression (e.g. `make_unique`)


================
Comment at: lld/ELF/Propeller.cpp:338
+            auto T = N->ShName.find_first_of('.');
+            if (T != string::npos && T == NumOnes) {
+              return N.get();
----------------
Omit `{}` for simple statements.


================
Comment at: lld/ELF/Propeller.h:1
+#ifndef LLD_ELF_PROPELLER_H
+#define LLD_ELF_PROPELLER_H
----------------
Add license notice.
Use variableName instead of VariableName to conform to the local naming convention.


================
Comment at: lld/ELF/Propeller.h:18
+
+using llvm::StringRef;
+
----------------
Delete it. Common llvm types are exported by include/lld/Common/LLVM.h which is transitively included in most headers.


================
Comment at: lld/ELF/Propeller.h:20
+
+using std::list;
+using std::map;
----------------
Delete such `using std::$container;`


================
Comment at: lld/ELF/Propeller.h:113
+public:
+  Propfile(FILE *PS, Propeller &P)
+      : BPAllocator(), PropfileStrSaver(BPAllocator), PStream(PS), Prop(P),
----------------
Use MemoryBuffer instead of FILE*.

Consider elf::readFile or

```
  auto mbOrErr = MemoryBuffer::getFile(path, -1, false);
```


================
Comment at: lld/ELF/Propeller.h:118
+    // malloc.
+    LineBuf = (char *)malloc(LineSize);
+  }
----------------
Prefer a vector to a malloc allocated buffer.


================
Comment at: lld/ELF/Propeller.h:136
+                                    uint64_t Size) {
+    auto *Sym = new SymbolEntry(Ordinal, Name, std::move(Aliases),
+                                SymbolEntry::INVALID_ADDRESS, Size,
----------------
Prefer unique_ptr to raw pointers.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062