Default arguments considered harmful?

This got me thinking, perhaps it's time to introduce coding style policy to discourage excessive use of default arguments?

Check out this gem from ASTUnit.h:

   static ASTUnit *LoadFromCommandLine(
       const char **ArgBegin, const char **ArgEnd,
       IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef ResourceFilesPath,
       bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
       ArrayRef<RemappedFile> RemappedFiles = None,
       bool RemappedFilesKeepOriginalName = true,
       bool PrecompilePreamble = false, TranslationUnitKind TUKind = TU_Complete,
       bool CacheCodeCompletionResults = false,
       bool IncludeBriefCommentsInCodeCompletion = false,
       bool AllowPCHWithCompilerErrors = false, bool SkipFunctionBodies = false,
       bool UserFilesAreVolatile = false, bool ForSerialization = false,
       std::unique_ptr<ASTUnit> *ErrAST = 0);

Alp.

This got me thinking, perhaps it's time to introduce coding style policy to
discourage excessive use of default arguments?

Check out this gem from ASTUnit.h:

  static ASTUnit *LoadFromCommandLine(
      const char **ArgBegin, const char **ArgEnd,
      IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef
ResourceFilesPath,
      bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
      ArrayRef<RemappedFile> RemappedFiles = None,
      bool RemappedFilesKeepOriginalName = true,
      bool PrecompilePreamble = false, TranslationUnitKind TUKind =
TU_Complete,
      bool CacheCodeCompletionResults = false,
      bool IncludeBriefCommentsInCodeCompletion = false,
      bool AllowPCHWithCompilerErrors = false, bool SkipFunctionBodies =
false,
      bool UserFilesAreVolatile = false, bool ForSerialization = false,
      std::unique_ptr<ASTUnit> *ErrAST = 0);

From this I would /probably/ (but maybe not) be more inclined to

conclude "bool arguments considered harmful" - the readability of the
callers is hurt mostly by the bool arguments (their the ones that need
comments, regardless of them being defaulted or not).

And possibly: excessive arguments considered harmful (having 20+
arguments is unlikely to be the best idea... )

> This got me thinking, perhaps it's time to introduce coding style policy
to
> discourage excessive use of default arguments?
>
> Check out this gem from ASTUnit.h:
>
> static ASTUnit *LoadFromCommandLine(
> const char **ArgBegin, const char **ArgEnd,
> IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef
> ResourceFilesPath,
> bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
> ArrayRef<RemappedFile> RemappedFiles = None,
> bool RemappedFilesKeepOriginalName = true,
> bool PrecompilePreamble = false, TranslationUnitKind TUKind =
> TU_Complete,
> bool CacheCodeCompletionResults = false,
> bool IncludeBriefCommentsInCodeCompletion = false,
> bool AllowPCHWithCompilerErrors = false, bool SkipFunctionBodies =
> false,
> bool UserFilesAreVolatile = false, bool ForSerialization = false,
> std::unique_ptr<ASTUnit> *ErrAST = 0);

Yikes! But ASTRecordLayout's constructor beats this function's puny 17
parameters -- it has 20. And DeclaratorChunk::getFunction is the winner in
Clang, with 21 parameters (and only one miserable, lonely default
argument). In total, LLVM and Clang (and LLDB and whatever else is in my
source tree...) has nearly 100 functions in header files with 10 or more
parameters.

From this I would /probably/ (but maybe not) be more inclined to
conclude "bool arguments considered harmful" - the readability of the
callers is hurt mostly by the bool arguments (their the ones that need
comments, regardless of them being defaulted or not).

And possibly: excessive arguments considered harmful (having 20+
arguments is unlikely to be the best idea... )

+1 to both of these. Also of note: that function is called once in the
entire codebase. The default arguments are never used.

    > This got me thinking, perhaps it's time to introduce coding
    style policy to
    > discourage excessive use of default arguments?
    >
    > Check out this gem from ASTUnit.h:
    >
    > static ASTUnit *LoadFromCommandLine(
    > const char **ArgBegin, const char **ArgEnd,
    > IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef
    > ResourceFilesPath,
    > bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
    > ArrayRef<RemappedFile> RemappedFiles = None,
    > bool RemappedFilesKeepOriginalName = true,
    > bool PrecompilePreamble = false, TranslationUnitKind TUKind =
    > TU_Complete,
    > bool CacheCodeCompletionResults = false,
    > bool IncludeBriefCommentsInCodeCompletion = false,
    > bool AllowPCHWithCompilerErrors = false, bool
    SkipFunctionBodies =
    > false,
    > bool UserFilesAreVolatile = false, bool ForSerialization =
    false,
    > std::unique_ptr<ASTUnit> *ErrAST = 0);

Yikes! But ASTRecordLayout's constructor beats this function's puny 17 parameters -- it has 20. And DeclaratorChunk::getFunction is the winner in Clang, with 21 parameters (and only one miserable, lonely default argument). In total, LLVM and Clang (and LLDB and whatever else is in my source tree...) has nearly 100 functions in header files with 10 or more parameters.

Wow.

    From this I would /probably/ (but maybe not) be more inclined to
    conclude "bool arguments considered harmful" - the readability of the
    callers is hurt mostly by the bool arguments (their the ones that need
    comments, regardless of them being defaulted or not).

    And possibly: excessive arguments considered harmful (having 20+
    arguments is unlikely to be the best idea... )

+1 to both of these. Also of note: that function is called once in the entire codebase. The default arguments are never used.

Yes agree to both, with the possible observation that default arguments are "enablers" for those transgressions and others by letting them slip through unnoticed.

All three are poor form and they tend to begin as a quick-fix or feature-driven work that ends up causing pain to whoever is actually maintaining the code.

CC'ing in llvmdev (as I meant to do originally) in case there's interest amending the style guide.

Alp.

I wonder if, with C++11 named initialization syntax for PODs, something
could be done so that in cases like this a (function specific, I guess)
"optional options object" could be used. (This would avoid one of the big
problems with C++ optional arguments, which is that if one towards the end
needs to be set to a non-default value all the preceding options need
setting.) That would certainly make things a lot easier to read in cases
like these.

Ah forget that. It's been a while since I tried these games and remember now
why it doesn't work.

The related issue is that they complicate overloading. I've been bitten a few times in clang code by the compiler deciding that, when I specified a pointer instead of a reference for a parameter, it would happily coerce it to a bool and then use the default values for the rest of the parameters, giving some very strange results - no errors, no warnings, just the code doing something unexpected.

David

David, I think you're pointing out a slightly different issue... one I've also been bitten by.

Does anyone know of a legitimate reason to have an implicit conversion to bool as a function argument? Maybe we should add an optional warning for this case in particular?

Philip

My suspicion is that this would (just barely) not meet our bar for
false positive rate. (my bet, though it's not a sure thing, is that we
do this in the LLVM project at least tens of times intentionally (ie:
not a bug, but subtle code))

But, arguably, rewriting the argument from "func(x)" to "func(x !=
nullptr)" is a benefit in readability and sometimes we're OK with a
few more false positives if they're just "badly written/confusing
code"

- David

>
>>
>>
>>> I wonder if, with C++11 named initialization syntax for PODs, something
>>> could be done so that in cases like this a (function specific, I guess)
>>> "optional options object" could be used. (This would avoid one of the
big
>>> problems with C++ optional arguments, which is that if one towards the
>>> end
>>> needs to be set to a non-default value all the preceding options need
>>> setting.) That would certainly make things a lot easier to read in
cases
>>> like these.
>>
>> The related issue is that they complicate overloading. I've been
bitten a
>> few times in clang code by the compiler deciding that, when I specified
a
>> pointer instead of a reference for a parameter, it would happily coerce
it
>> to a bool and then use the default values for the rest of the
parameters,
>> giving some very strange results - no errors, no warnings, just the code
>> doing something unexpected.
>>
>> David
>>
> David, I think you're pointing out a slightly different issue... one I've
> also been bitten by.
>
> Does anyone know of a legitimate reason to have an implicit conversion to
> bool as a function argument? Maybe we should add an optional warning for
> this case in particular?

My suspicion is that this would (just barely) not meet our bar for
false positive rate. (my bet, though it's not a sure thing, is that we
do this in the LLVM project at least tens of times intentionally (ie:
not a bug, but subtle code))

But, arguably, rewriting the argument from "func(x)" to "func(x !=
nullptr)" is a benefit in readability and sometimes we're OK with a
few more false positives if they're just "badly written/confusing
code"

I feel like this warning would be quite useful/valuable if it is (or can
be) limited to implicit bool conversions for optional arguments. Though
like you, I suspect warning on implicit bool conversions even for required
arguments may not fall under the threshold for false positives.

Cheers,
Kaelyn

Well, record layout is complicated. :slight_smile: Would you rather build a fresh
record layout and set the fields individually, potentially letting callers
ignore some fields?

In article <CACs=tyJfAHEMvKY2y_e22fnDjVXeHoDC4n5B=9KkrQAeyOU1Zg@mail.gmail.com>,
    Reid Kleckner <rnk@google.com> writes:

> Yikes! But ASTRecordLayout's constructor beats this function's puny 17
> parameters -- it has 20. And DeclaratorChunk::getFunction is the winner in
> Clang, with 21 parameters (and only one miserable, lonely default
> argument). In total, LLVM and Clang (and LLDB and whatever else is in my
> source tree...) has nearly 100 functions in header files with 10 or more
> parameters.
>

Well, record layout is complicated. :slight_smile: Would you rather build a fresh
record layout and set the fields individually, potentially letting callers
ignore some fields?

When I have lots of parameters that control creating an object, I tend
to prefer a "builder" interface that gives me readable code,
reasonable defaults and avoids the long parameter smell.

Here's an example I wrote up for describing GPU resource parameters in
the Direct3D11 API. These GPU resources have lots of parameters (width,
height, bit plane depth) and lots of options, many of which have reasonable
defaults.
<http://legalizeadulthood.wordpress.com/2009/07/12/description-helpers-for-direct3d-10-10-1-and-11/>

This only works if you have reasonable defaults for all of the parameters
(see Reid's comments). If there is a mixture of reasonable defaults and
important parameters, a builder which requires the important ones in the
constructor and allows you to override the defaults of the rest is quite
reasonable though. It isn't clear whether this is such a case, but it might
be.