[PATCH] D73497: lld: Propeller framework part I

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

The variable naming does not follow lld's convention (lowerCase).



================
Comment at: lld/ELF/Propeller/PropellerConfig.h:12
+
+using llvm::StringRef;
+
----------------
`lld/Common/LLVM.h` pulls in declarations of common utilities. No need for `using llvm::StringRef`.


================
Comment at: lld/include/lld/Common/PropellerCommon.h:17
+static const char BASIC_BLOCK_SEPARATOR[] = ".BB.";
+static const char *BASIC_BLOCK_UNIFIED_CHARACTERS = "arlL";
+
----------------
Prefer `[]`


================
Comment at: lld/include/lld/Common/PropellerCommon.h:28
+struct SymbolEntry {
+
+  enum BBTagTypeEnum : unsigned char {
----------------
excess empty line


================
Comment at: lld/include/lld/Common/PropellerCommon.h:46
+  // Unique index number across all symbols that participate linking.
+  uint64_t Ordinal;
+  // For a function symbol, it's the full name. For a bb symbol this is only the
----------------
Prefer lowerCased variable names.


================
Comment at: lld/include/lld/Common/PropellerCommon.h:82
+    if (O->Size == 0) {
+      // Note if O's size is 0, we allow O on the end boundary. For example, if
+      // foo.BB.4 is at address 0x10. foo is [0x0, 0x10), we then assume foo
----------------
Rationale? Do you have a concrete example this rule is needed?


================
Comment at: lld/include/lld/Common/PropellerCommon.h:95
+  bool isFunction() const {
+    return this->Type == llvm::object::SymbolRef::ST_Function;
+  }
----------------
Drop unnecessary `this`


================
Comment at: lld/include/lld/Common/PropellerCommon.h:152
+struct SymbolEntryOrdinalLessComparator {
+  bool operator()(SymbolEntry *S1, SymbolEntry *S2) const {
+    if (!S1 && S2)
----------------
```lang=cpp
if (s1 && s2)
  return s1->ordinal < s2->ordinal;
return !!s1 < !!s2;
```


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

https://reviews.llvm.org/D73497