jasonliu added inline comments.

Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:391
+  /// @param[out] Structors Sorted Structor structs by Priority.
+  /// @return false if List is not an array of '{ i32, void ()*, i8* }' 
+  bool preprocessXXStructorList(const DataLayout &DL, const Constant *List,
This description is not entirely true. We only see if the array is a 
ConstantArray and returning false. We are not returning false if the array's 
element is not `{ i32, void ()*, i8* }`.

Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466
+  bool preprocessStructorList(const DataLayout &DL, const Constant *List,
+                              SmallVector<Structor, 8> &Structors);
Xiangling_L wrote:
> jasonliu wrote:
> > A doxygen comment describe what this function does, and what its return 
> > value means, and mention `Structors` is an output argument.
> > By looking at what this function does, it seems `buildStructorList` is a 
> > better name.
> I meant to and should've named this function to `preprocessXXStructorList`, 
> let me know if you still prefer `buildStructorList`. And if you do, since the 
> underneath of `SmallVector` is a variable-sized array, maybe we should try 
> `buildSortedStructorArray`?
`preprocess` sounds like we are already having a XXStructorList and now we try 
to do something on it. 
But right now, we are actually passing in an empty StructorList/Array and build 
it from scratch. So I would still prefer the name of `build` in it.
I don't mind changing to a more accurate name as you suggested. 

Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107
+  if (!isa<ConstantArray>(List))
+    return false;
Return of boolean seems unnecessary. 
Callee could check the size of the Structors to decide if they want an early 
return or not (or in this particular case, the for loop would just do nothing 
and no need for extra condition if you don't mind the call to 
getPointerPrefAlignment or assign to 0 to Index)?
So we could just return void for this function?

Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2129
   // Emit the function pointers in the target-specific order
   llvm::stable_sort(Structors, [](const Structor &L, const Structor &R) {
nit: Missing blank line.

Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1966
+    Index++;
+  }
nit: Use `++Index` instead. 

Or use `Index++` at line 1963 so that we don't need this line.



cfe-commits mailing list

Reply via email to