Xiangling_L marked 5 inline comments as done. Xiangling_L added inline comments.
================ Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:466 + + bool preprocessStructorList(const DataLayout &DL, const Constant *List, + SmallVector<Structor, 8> &Structors); ---------------- jasonliu wrote: > 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. I think we do have a `XXStructorList` here which is the initializer of `llvm.gloal_ctors`or `llvm.gloal_dtors` array? The usage of this term is consistent with other spots. ================ Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107 + if (!isa<ConstantArray>(List)) + return false; ---------------- jasonliu wrote: > 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? Yeah, we could do that. But it looks a boolean return value makes the code flow natural. And if any target does want to control the early return in the future, it's flexbile for them to do that. I am wondering is there any big difference between bool and void return value for this function? 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
