llvm dev - Feb 2019 - Specify default value for FunctionType::get's isVarArg parameter

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<Type*> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190206/77e2809a/attachment.html>

Personally, I'd rather to delete the overload lacking "Params",
and
otherwise leave it as is. Make all callers always specify all three
arguments.

", {}, false" is really not a lot of clutter.

On Wed, Feb 6, 2019, 10:08 PM Julian Lettner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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<Type*>
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 at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190207/270c02f8/attachment.html>