jasonliu added inline comments.
================ 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: > > 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. My understanding is that before we enter this `preprocessXXStructorList`, we do not have a list of XXStructor. We only have a list of `Constant`. This functions turns a list of `Constant` to a list of `XXStructor`. ================ Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2107 + if (!isa<ConstantArray>(List)) + return false; ---------------- Xiangling_L wrote: > 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? If target need to control early return in the future, they could still do so by querying if the output `Structors` is empty or not. I just don't want unnecessary returns, as it's one more thing that user of the function need to care about when they examine this function. And the user of this function would have the same question of why we need to return a boolean here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84534/new/ https://reviews.llvm.org/D84534 _______________________________________________ cfe-commits mailing list email@example.com https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits