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* }'
structs.
+ 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.
https://llvm.org/docs/CodingStandards.html#prefer-preincrement
Or use `Index++` at line 1963 so that we don't need this line.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84534/new/
https://reviews.llvm.org/D84534
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
