[cfe-dev] Default arguments considered harmful?

David Blaikie dblaikie at gmail.com
Thu May 1 21:02:31 PDT 2014


On Thu, May 1, 2014 at 8:57 PM, Alp Toker <alp at 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);

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

>
>
> 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 at 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 at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits



More information about the cfe-dev mailing list