<div dir="auto"><div>Personally, I'd rather to delete the overload lacking "Params", and otherwise leave it as is. Make all callers always specify all three arguments.</div><div dir="auto"><br></div><div dir="auto">", {}, false" is really not a lot of clutter.<br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Wed, Feb 6, 2019, 10:08 PM Julian Lettner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Typical usage (good citizens annotate their boolean args):</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><font face="Courier New">… = FunctionType::get(IRB.getVoidTy(), {IntptrTy, IntptrTy}, /*isVarArg=*/ false);</font></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Level 0: Add default in declaration/header</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space">Enables simplified use in the future and gradual cleanup.</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Level 1: Adapt call sites</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Cons: This cleanup is not important enough to warrant the churn.</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space">This requires at least a cleanup of the 2-parameter overload usages.</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space">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;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Level 3/orthogonal bonus level: Special getter for void *(void) function type</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">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;line-break:after-white-space">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;line-break:after-white-space">We can also have a separate discussion for this since it is orthogonal.</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">What do you think? Are there other cons I have overlooked?</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space">Which level would you like to see?</div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br></div><div dir="auto" style="word-wrap:break-word;line-break:after-white-space"><br><div>Approximate counts (the regex I used are probably not 100% accurate):</div><div><br></div><div><div><font face="Courier New">+--------+-------+------+----------+</font></div><div><font face="Courier New">| | false | true | variable |</font></div><div><font face="Courier New">+--------+-------+------+----------+</font></div><div><font face="Courier New">| 3 args | 338 | 14 | 15 |</font></div><div><font face="Courier New">| 2 args | 86 | 7 | 9 |</font></div><div><font face="Courier New">+--------+-------+------+----------+</font></div></div><div><font face="Courier New"><br></font></div><div><span>APIs we are talking about:</span></div><div><div><div><font face="Courier New"><br></font></div><div><font face="Courier New">FunctionType *FunctionType::get(Type *ReturnType, ArrayRef<Type*> Params, bool isVarArg) {</font></div></div><div><font face="Courier New"> // Lookup; create if not exists</font></div><div><font face="Courier New">}</font></div><div><font face="Courier New"><br></font></div><div><div><font face="Courier New">FunctionType *FunctionType::get(Type *Result, bool isVarArg) {</font></div><div><font face="Courier New"> return get(Result, None, isVarArg);</font></div><div><font face="Courier New">}</font></div></div></div><div><font face="Courier New"><br></font></div><div><font face="Courier New"><br></font></div><span><span><span>Regex:<br><br><font face="Courier New">➤ rg "FunctionType::get\(" | wc -l<br> 612 # all, including multiline, which are not included below<br>➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*\)" | wc -l<br> 367 # 3 params<br>➤ rg "FunctionType::get\([^,]+,[^,]*\)" | wc -l<br> 102 # 2 params<br><br>➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*false[^,]*\)" | wc -l<br> 338 # 3 params, false<br>➤ rg "FunctionType::get\([^,]+,[^,]+,[^,]*true[^,]*\)" | wc -l<br> 14 # 3 params, true<br><br>➤ rg "FunctionType::get\([^,]+,[^,]*false[^,]*\)" | wc -l<br> 86 # 2 params, false<br>➤ rg "FunctionType::get\([^,]+,[^,]*true[^,]*\)" | wc -l<br> 7 # 2 params, true<br><br>➤ rg "FunctionType::get\([^,]*[Vv]oid[^,]*,[^,]*false[^,]*\)" | wc -l<br> 77 # void (void) function type</font><br><br></span></span></span></div>
</div></div></div></div>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" rel="noreferrer">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div></div>