Re: [cfe-dev] Default arguments considered harmful?

[Alp Toker]

On 02/05/2014 05:26, Richard Smith wrote:
On Thu, May 1, 2014 at 9:02 PM, David Blaikie <dblai...@gmail.com 
<mailto:dblai...@gmail.com>> wrote:

    On Thu, May 1, 2014 at 8:57 PM, Alp Toker <a...@nuanti.com
    <mailto:a...@nuanti.com>> wrote:
    > 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.



    > Alp.
    >
    >
    > On 02/05/2014 04:43, Alp Toker wrote:
    >>
    >> Author: alp
    >> Date: Thu May  1 22:43:30 2014
    >> New Revision: 207825
    >>
    >> URL: http://llvm.org/viewvc/llvm-project?rev=207825&view=rev
    >> Log:
    >> Factor TargetInfo pointer/DelayInitialization bool pair out of
    >> Preprocessor ctor
    >>
    >> The Preprocessor::Initialize() function already offers a clear
    interface
    >> to
    >> achieve this, further reducing the confusing number of states a
    newly
    >> constructed preprocessor can have.
    >>
    >> Modified:
    >>  cfe/trunk/include/clang/Lex/Preprocessor.h
    >>      cfe/trunk/lib/Frontend/ASTUnit.cpp
    >>  cfe/trunk/lib/Frontend/CompilerInstance.cpp
    >>      cfe/trunk/lib/Lex/Preprocessor.cpp
    >>  cfe/trunk/unittests/Basic/SourceManagerTest.cpp
    >>      cfe/trunk/unittests/Lex/LexerTest.cpp
    >>  cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
    >>  cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
    >>
    >> Modified: cfe/trunk/include/clang/Lex/Preprocessor.h
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/include/clang/Lex/Preprocessor.h (original)
    >> +++ cfe/trunk/include/clang/Lex/Preprocessor.h Thu May  1
    22:43:30 2014
    >> @@ -455,12 +455,10 @@ private:  // Cached tokens state.
    >>   public:
    >> Preprocessor(IntrusiveRefCntPtr<PreprocessorOptions> PPOpts,
    >>                  DiagnosticsEngine &diags, LangOptions &opts,
    >> -               const TargetInfo *target,
    >>                  SourceManager &SM, HeaderSearch &Headers,
    >>                  ModuleLoader &TheModuleLoader,
    >>                  IdentifierInfoLookup *IILookup = 0,
    >>                  bool OwnsHeaderSearch = false,
    >> -               bool DelayInitialization = false,
    >>                  TranslationUnitKind TUKind = TU_Complete);
    >>       ~Preprocessor();
    >>
    >> Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
    >> +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Thu May  1 22:43:30 2014
    >> @@ -718,11 +718,10 @@ ASTUnit *ASTUnit::LoadFromASTFile(const
    >>       AST->PP = new Preprocessor(PPOpts,
    >>  AST->getDiagnostics(),
    >> AST->ASTFileLangOpts,
    >> -                             /*Target=*/0,
    AST->getSourceManager(),
    >> HeaderInfo,
    >> + AST->getSourceManager(), HeaderInfo,
    >>                                *AST,
    >>  /*IILookup=*/0,
    >> - /*OwnsHeaderSearch=*/false,
    >> - /*DelayInitialization=*/true);
    >> + /*OwnsHeaderSearch=*/false);
    >>     Preprocessor &PP = *AST->PP;
    >>       AST->Ctx = new ASTContext(AST->ASTFileLangOpts,
    >>
    >> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
    >> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Thu May  1
    22:43:30 2014
    >> @@ -240,11 +240,11 @@ void CompilerInstance::createPreprocesso
    >>   getLangOpts(),
    >>   &getTarget());
    >>     PP = new Preprocessor(&getPreprocessorOpts(),
    >> -                        getDiagnostics(), getLangOpts(),
    &getTarget(),
    >> +                        getDiagnostics(), getLangOpts(),
    >>                           getSourceManager(), *HeaderInfo,
    *this, PTHMgr,
    >> /*OwnsHeaderSearch=*/true,
    >> -  /*DelayInitialization=*/false,
    >>                           TUKind);
    >> +  PP->Initialize(getTarget());
    >>       // Note that this is different then passing PTHMgr to
    Preprocessor's
    >> ctor.
    >>     // That argument is used as the IdentifierInfoLookup
    argument to
    >>
    >> Modified: cfe/trunk/lib/Lex/Preprocessor.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/Preprocessor.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/lib/Lex/Preprocessor.cpp (original)
    >> +++ cfe/trunk/lib/Lex/Preprocessor.cpp Thu May  1 22:43:30 2014
    >> @@ -56,11 +56,11 @@ ExternalPreprocessorSource::~ExternalPre
    >> Preprocessor::Preprocessor(IntrusiveRefCntPtr<PreprocessorOptions>
    >> PPOpts,
    >>  DiagnosticsEngine &diags, LangOptions &opts,
    >> -                           const TargetInfo *target,
    SourceManager &SM,
    >> +                           SourceManager &SM,
    >>                              HeaderSearch &Headers, ModuleLoader
    >> &TheModuleLoader,
    >>  IdentifierInfoLookup *IILookup, bool
    >> OwnsHeaders,
    >> -                           bool DelayInitialization,
    TranslationUnitKind
    >> TUKind)
    >> -    : PPOpts(PPOpts), Diags(&diags), LangOpts(opts),
    Target(target),
    >> + TranslationUnitKind TUKind)
    >> +    : PPOpts(PPOpts), Diags(&diags), LangOpts(opts), Target(0),
    >>         FileMgr(Headers.getFileMgr()), SourceMgr(SM),
    HeaderInfo(Headers),
    >>         TheModuleLoader(TheModuleLoader), ExternalSource(0),
    >>         Identifiers(opts, IILookup), IncrementalProcessing(false),
    >> @@ -132,11 +132,6 @@ Preprocessor::Preprocessor(IntrusiveRefC
    >>       Ident___exception_info = Ident___exception_code =
    >> Ident___abnormal_termination = 0;
    >>       Ident_GetExceptionInfo = Ident_GetExceptionCode =
    >> Ident_AbnormalTermination = 0;
    >>     }
    >> -
    >> -  if (!DelayInitialization) {
    >> -    assert(Target && "Must provide target information for PP
    >> initialization");
    >> -    Initialize(*Target);
    >> -  }
    >>   }
    >>     Preprocessor::~Preprocessor() {
    >>
    >> Modified: cfe/trunk/unittests/Basic/SourceManagerTest.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/SourceManagerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/unittests/Basic/SourceManagerTest.cpp (original)
    >> +++ cfe/trunk/unittests/Basic/SourceManagerTest.cpp Thu May  1
    22:43:30
    >> 2014
    >> @@ -80,11 +80,11 @@ TEST_F(SourceManagerTest, isBeforeInTran
    >>     VoidModuleLoader ModLoader;
    >>     HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr,
    Diags,
    >> LangOpts,
    >>                             &*Target);
    >> -  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >> Target.getPtr(),
    >> +  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >>                     SourceMgr, HeaderInfo, ModLoader,
    >>                     /*IILookup =*/ 0,
    >> -                  /*OwnsHeaderSearch =*/false,
    >> -                  /*DelayInitialization =*/ false);
    >> +                  /*OwnsHeaderSearch =*/false);
    >> +  PP.Initialize(*Target);
    >>     PP.EnterMainSourceFile();
    >>       std::vector<Token> toks;
    >> @@ -195,11 +195,11 @@ TEST_F(SourceManagerTest, getMacroArgExp
    >>     VoidModuleLoader ModLoader;
    >>     HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr,
    Diags,
    >> LangOpts,
    >>                             &*Target);
    >> -  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >> Target.getPtr(),
    >> +  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >>                     SourceMgr, HeaderInfo, ModLoader,
    >>                     /*IILookup =*/ 0,
    >> -                  /*OwnsHeaderSearch =*/false,
    >> -                  /*DelayInitialization =*/ false);
    >> +                  /*OwnsHeaderSearch =*/false);
    >> +  PP.Initialize(*Target);
    >>     PP.EnterMainSourceFile();
    >>       std::vector<Token> toks;
    >> @@ -293,11 +293,11 @@ TEST_F(SourceManagerTest, isBeforeInTran
    >>     VoidModuleLoader ModLoader;
    >>     HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr,
    Diags,
    >> LangOpts,
    >>                             &*Target);
    >> -  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >> Target.getPtr(),
    >> +  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >>                     SourceMgr, HeaderInfo, ModLoader,
    >>                     /*IILookup =*/ 0,
    >> -                  /*OwnsHeaderSearch =*/false,
    >> -                  /*DelayInitialization =*/ false);
    >> +                  /*OwnsHeaderSearch =*/false);
    >> +  PP.Initialize(*Target);
    >>       std::vector<MacroAction> Macros;
    >>     PP.addPPCallbacks(new MacroTracker(Macros));
    >>
    >> Modified: cfe/trunk/unittests/Lex/LexerTest.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/LexerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/unittests/Lex/LexerTest.cpp (original)
    >> +++ cfe/trunk/unittests/Lex/LexerTest.cpp Thu May  1 22:43:30 2014
    >> @@ -69,10 +69,10 @@ protected:
    >>       VoidModuleLoader ModLoader;
    >>       HeaderSearch HeaderInfo(new HeaderSearchOptions,
    SourceMgr, Diags,
    >> LangOpts,
    >> Target.getPtr());
    >> -    Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >> Target.getPtr(),
    >> +    Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >>                       SourceMgr, HeaderInfo, ModLoader,
    /*IILookup =*/ 0,
    >> -                    /*OwnsHeaderSearch =*/ false,
    >> -                    /*DelayInitialization =*/ false);
    >> +                    /*OwnsHeaderSearch =*/ false);
    >> +    PP.Initialize(*Target);
    >>       PP.EnterMainSourceFile();
    >>         std::vector<Token> toks;
    >>
    >> Modified: cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/unittests/Lex/PPCallbacksTest.cpp (original)
    >> +++ cfe/trunk/unittests/Lex/PPCallbacksTest.cpp Thu May  1
    22:43:30 2014
    >> @@ -173,11 +173,10 @@ protected:
    >> IntrusiveRefCntPtr<PreprocessorOptions> PPOpts = new
    >> PreprocessorOptions();
    >>       Preprocessor PP(PPOpts, Diags, LangOpts,
    >> -      Target.getPtr(),
    >>         SourceMgr, HeaderInfo, ModLoader,
    >>         /*IILookup =*/ 0,
    >> -      /*OwnsHeaderSearch =*/false,
    >> -      /*DelayInitialization =*/ false);
    >> +      /*OwnsHeaderSearch =*/false);
    >> +    PP.Initialize(*Target);
    >>       InclusionDirectiveCallbacks* Callbacks = new
    >> InclusionDirectiveCallbacks;
    >>       PP.addPPCallbacks(Callbacks); // Takes ownership.
    >>   @@ -208,11 +207,10 @@ protected:
    >>                               OpenCLLangOpts, Target.getPtr());
    >>         Preprocessor PP(new PreprocessorOptions(), Diags,
    OpenCLLangOpts,
    >> -                    Target.getPtr(),
    >>                       SourceMgr, HeaderInfo, ModLoader,
    >>                      /*IILookup =*/ 0,
    >> -                    /*OwnsHeaderSearch =*/false,
    >> -                    /*DelayInitialization =*/ false);
    >> +                    /*OwnsHeaderSearch =*/false);
    >> +    PP.Initialize(*Target);
    >>         // parser actually sets correct pragma handlers for
    preprocessor
    >>       // according to LangOptions, so we init Parser to
    register opencl
    >>
    >> Modified:
    cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
    >> URL:
    >>
    
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp?rev=207825&r1=207824&r2=207825&view=diff
    >>
    >>
    
==============================================================================
    >> --- cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
    >> (original)
    >> +++
    cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Thu May
    >> 1 22:43:30 2014
    >> @@ -97,11 +97,11 @@ TEST_F(PPConditionalDirectiveRecordTest,
    >>     VoidModuleLoader ModLoader;
    >>     HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr,
    Diags,
    >> LangOpts,
    >>                             Target.getPtr());
    >> -  Preprocessor PP(new PreprocessorOptions(), Diags,
    >> LangOpts,Target.getPtr(),
    >> +  Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,
    >>                     SourceMgr, HeaderInfo, ModLoader,
    >>                     /*IILookup =*/ 0,
    >> -                  /*OwnsHeaderSearch =*/false,
    >> -                  /*DelayInitialization =*/ false);
    >> +                  /*OwnsHeaderSearch =*/false);
    >> +  PP.Initialize(*Target);
    >>     PPConditionalDirectiveRecord *
    >>       PPRec = new PPConditionalDirectiveRecord(SourceMgr);
    >>     PP.addPPCallbacks(PPRec);
    >>
    >>
    >> _______________________________________________
    >> cfe-commits mailing list
    >> cfe-commits@cs.uiuc.edu <mailto:cfe-commits@cs.uiuc.edu>
    >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
    >
    >
    > --
    > http://www.nuanti.com
    > the browser experts
    >
    > _______________________________________________
    > cfe-commits mailing list
    > cfe-commits@cs.uiuc.edu <mailto:cfe-commits@cs.uiuc.edu>
    > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
    _______________________________________________
    cfe-dev mailing list
    cfe-...@cs.uiuc.edu <mailto:cfe-...@cs.uiuc.edu>
    http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



--
http://www.nuanti.com
the browser experts

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits