(original) (raw)

Should we specify a default value (false) for the isVarArg parameter of the FunctionType::get functions to improve ergonomics and reduce visual noise?

Typical usage (good citizens annotate their boolean args):

… = FunctionType::get(IRB.getVoidTy(), {IntptrTy, IntptrTy}, /\*isVarArg=\*/ false);

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.

There are different levels for the proposed change. I also state all the cons that I could think of.

Level 0: Add default in declaration/header

Cons: No, we want to require users to specify isVarArg, i.e., force them to think about the varargs special case.

Enables simplified use in the future and gradual cleanup.

Level 1: Adapt call sites

Cons: This cleanup is not important enough to warrant the churn.

Level 2: Get rid of 2-parameter overload by specifying an additional default in the 3-parameter variant: Params = None

This requires at least a cleanup of the 2-parameter overload usages.

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).

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.

Level 3/orthogonal bonus level: Special getter for void \*(void) function type

77 out of 102 usages of the 2-parameter variant are used to retrieve a void func(void) type!

Maybe this warrants a special getter, similar to Type::getInt32Ty, for 1) convenience and 2) avoiding the lookup (eagerly initialize it).

We can also have a separate discussion for this since it is orthogonal.

What do you think? Are there other cons I have overlooked?

Which level would you like to see?

Approximate counts (the regex I used are probably not 100% accurate):

+--------+-------+------+----------+

| | false | true | variable |

+--------+-------+------+----------+

| 3 args | 338 | 14 | 15 |

| 2 args | 86 | 7 | 9 |

+--------+-------+------+----------+

APIs we are talking about:

FunctionType \*FunctionType::get(Type \*ReturnType, ArrayRef Params, bool isVarArg) {

// Lookup; create if not exists

}

FunctionType \*FunctionType::get(Type \*Result, bool isVarArg) {

return get(Result, None, isVarArg);

}

Regex:

➤ rg "FunctionType::get\\(" | wc -l
612 # all, including multiline, which are not included below
➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]+,\[^,\]\*\\)" | wc -l
367 # 3 params
➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]\*\\)" | wc -l
102 # 2 params

➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]+,\[^,\]\*false\[^,\]\*\\)" | wc -l
338 # 3 params, false
➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]+,\[^,\]\*true\[^,\]\*\\)" | wc -l
14 # 3 params, true

➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]\*false\[^,\]\*\\)" | wc -l
86 # 2 params, false
➤ rg "FunctionType::get\\(\[^,\]+,\[^,\]\*true\[^,\]\*\\)" | wc -l
7 # 2 params, true

➤ rg "FunctionType::get\\(\[^,\]\*\[Vv\]oid\[^,\]\*,\[^,\]\*false\[^,\]\*\\)" | wc -l
77 # void (void) function type

\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_\_
LLVM Developers mailing list
llvm-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev