<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Should we specify a default value (false) for the isVarArg parameter of the FunctionType::get functions to improve ergonomics and reduce visual noise?</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Typical usage (good citizens annotate their boolean args):</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><font face="Courier New" class="">… = FunctionType::get(IRB.getVoidTy(), {IntptrTy, IntptrTy}, /*isVarArg=*/ false);</font></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">For all usages that I could easily grep, i.e., single-line usages, true was only used 21 times. So at least mechanically, false seems to be a good default.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">There are different levels for the proposed change. I also state all the cons that I could think of.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Level 0: Add default in declaration/header</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Cons: No, we want to require users to specify isVarArg, i.e., force them to think about the varargs special case.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Enables simplified use in the future and gradual cleanup.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Level 1: Adapt call sites</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Cons: This cleanup is not important enough to warrant the churn.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Level 2: Get rid of 2-parameter overload by specifying an additional default in the 3-parameter variant: Params = None</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">This requires at least a cleanup of the 2-parameter overload usages.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Cons: In addition, this would force ~16 (2-args-true and 2-args-variable) uses to specify None for the params argument (to “gain access” to the isVarArg, i.e., third positional argument).</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">IMO, this is not a cons. No-parameter var-args functions (no format string!) seem special enough that being explicit about the absence of normal parameters feels okay to me.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Level 3/orthogonal bonus level: Special getter for void *(void) function type</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">77 out of 102 usages of the 2-parameter variant are used to retrieve a void func(void) type!</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Maybe this warrants a special getter, similar to Type::getInt32Ty, for 1) convenience and 2) avoiding the lookup (eagerly initialize it).</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">We can also have a separate discussion for this since it is orthogonal.</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">What do you think? Are there other cons I have overlooked?</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Which level would you like to see?</div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""></div><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div class="">Approximate counts (the regex I used are probably not 100% accurate):</div><div class=""><br class=""></div><div class=""><div class=""><font face="Courier New" class="">+--------+-------+------+----------+</font></div><div class=""><font face="Courier New" class="">| | false | true | variable |</font></div><div class=""><font face="Courier New" class="">+--------+-------+------+----------+</font></div><div class=""><font face="Courier New" class="">| 3 args | 338 | 14 | 15 |</font></div><div class=""><font face="Courier New" class="">| 2 args | 86 | 7 | 9 |</font></div><div class=""><font face="Courier New" class="">+--------+-------+------+----------+</font></div></div><div class=""><font face="Courier New" class=""><br class=""></font></div><div class=""><span class="">APIs we are talking about:</span></div><div class=""><div class=""><div class=""><font face="Courier New" class=""><br class=""></font></div><div class=""><font face="Courier New" class="">FunctionType *FunctionType::get(Type *ReturnType, ArrayRef<Type*> Params, bool isVarArg) {</font></div></div><div class=""><font face="Courier New" class=""> // Lookup; create if not exists</font></div><div class=""><font face="Courier New" class="">}</font></div><div class=""><font face="Courier New" class=""><br class=""></font></div><div class=""><div class=""><font face="Courier New" class="">FunctionType *FunctionType::get(Type *Result, bool isVarArg) {</font></div><div class=""><font face="Courier New" class=""> return get(Result, None, isVarArg);</font></div><div class=""><font face="Courier New" class="">}</font></div></div></div><div class=""><font face="Courier New" class=""><br class=""></font></div><div class=""><font face="Courier New" class=""><br class=""></font></div><span class=""><span class=""><span class="">Regex:<br class=""><br class=""><font face="Courier New" class="">➤ rg "FunctionType::get\(" | wc -l<br class=""> 612 # all, including multiline, which are not included below<br class="">➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*\)" | wc -l<br class=""> 367 # 3 params<br class="">➤ rg "FunctionType::get\([^,]+,[^,]*\)" | wc -l<br class=""> 102 # 2 params<br class=""><br class="">➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*false[^,]*\)" | wc -l<br class=""> 338 # 3 params, false<br class="">➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*true[^,]*\)" | wc -l<br class=""> 14 # 3 params, true<br class=""><br class="">➤ rg "FunctionType::get\([^,]+,[^,]*false[^,]*\)" | wc -l<br class=""> 86 # 2 params, false<br class="">➤ rg "FunctionType::get\([^,]+,[^,]*true[^,]*\)" | wc -l<br class=""> 7 # 2 params, true<br class=""><br class="">➤ rg "FunctionType::get\([^,]*[Vv]oid[^,]*,[^,]*false[^,]*\)" | wc -l<br class=""> 77 # void (void) function type</font><br class=""><br class=""></span></span></span></div>
</div></div></div></body></html>