<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 1, 2014 at 9:02 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On Thu, May 1, 2014 at 8:57 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:<br>
> This got me thinking, perhaps it's time to introduce coding style policy to<br>
> discourage excessive use of default arguments?<br>
><br>
> Check out this gem from ASTUnit.h:<br>
><br>
> static ASTUnit *LoadFromCommandLine(<br>
> const char **ArgBegin, const char **ArgEnd,<br>
> IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef<br>
> ResourceFilesPath,<br>
> bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,<br>
> ArrayRef<RemappedFile> RemappedFiles = None,<br>
> bool RemappedFilesKeepOriginalName = true,<br>
> bool PrecompilePreamble = false, TranslationUnitKind TUKind =<br>
> TU_Complete,<br>
> bool CacheCodeCompletionResults = false,<br>
> bool IncludeBriefCommentsInCodeCompletion = false,<br>
> bool AllowPCHWithCompilerErrors = false, bool SkipFunctionBodies =<br>
> false,<br>
> bool UserFilesAreVolatile = false, bool ForSerialization = false,<br>
> std::unique_ptr<ASTUnit> *ErrAST = 0);<br></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
</div>From this I would /probably/ (but maybe not) be more inclined to<br>
conclude "bool arguments considered harmful" - the readability of the<br>
callers is hurt mostly by the bool arguments (their the ones that need<br>
comments, regardless of them being defaulted or not).<br>
<br>
And possibly: excessive arguments considered harmful (having 20+<br>
arguments is unlikely to be the best idea... )</blockquote><div><br></div><div>+1 to both of these. Also of note: that function is called once in the entire codebase. The default arguments are never used.</div><div><br></div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
> Alp.<br>
><br>
><br>
> On 02/05/2014 04:43, Alp Toker wrote:<br>
>><br>
>> Author: alp<br>
>> Date: Thu May 1 22:43:30 2014<br>
>> New Revision: 207825<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=207825&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=207825&view=rev</a><br>
>> Log:<br>
>> Factor TargetInfo pointer/DelayInitialization bool pair out of<br>
>> Preprocessor ctor<br>
>><br>
>> The Preprocessor::Initialize() function already offers a clear interface<br>
>> to<br>
>> achieve this, further reducing the confusing number of states a newly<br>
>> constructed preprocessor can have.<br>
>><br>
>> Modified:<br>
>> cfe/trunk/include/clang/Lex/Preprocessor.h<br>
>> cfe/trunk/lib/Frontend/ASTUnit.cpp<br>
>> cfe/trunk/lib/Frontend/CompilerInstance.cpp<br>
>> cfe/trunk/lib/Lex/Preprocessor.cpp<br>
>> cfe/trunk/unittests/Basic/SourceManagerTest.cpp<br>
>> cfe/trunk/unittests/Lex/LexerTest.cpp<br>
>> cfe/trunk/unittests/Lex/PPCallbacksTest.cpp<br>
>> cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp<br>
>><br>
>> Modified: cfe/trunk/include/clang/Lex/Preprocessor.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/include/clang/Lex/Preprocessor.h (original)<br>
>> +++ cfe/trunk/include/clang/Lex/Preprocessor.h Thu May 1 22:43:30 2014<br>
>> @@ -455,12 +455,10 @@ private: // Cached tokens state.<br>
>> public:<br>
>> Preprocessor(IntrusiveRefCntPtr<PreprocessorOptions> PPOpts,<br>
>> DiagnosticsEngine &diags, LangOptions &opts,<br>
>> - const TargetInfo *target,<br>
>> SourceManager &SM, HeaderSearch &Headers,<br>
>> ModuleLoader &TheModuleLoader,<br>
>> IdentifierInfoLookup *IILookup = 0,<br>
>> bool OwnsHeaderSearch = false,<br>
>> - bool DelayInitialization = false,<br>
>> TranslationUnitKind TUKind = TU_Complete);<br>
>> ~Preprocessor();<br>
>><br>
>> Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)<br>
>> +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Thu May 1 22:43:30 2014<br>
>> @@ -718,11 +718,10 @@ ASTUnit *ASTUnit::LoadFromASTFile(const<br>
>> AST->PP = new Preprocessor(PPOpts,<br>
>> AST->getDiagnostics(),<br>
>> AST->ASTFileLangOpts,<br>
>> - /*Target=*/0, AST->getSourceManager(),<br>
>> HeaderInfo,<br>
>> + AST->getSourceManager(), HeaderInfo,<br>
>> *AST,<br>
>> /*IILookup=*/0,<br>
>> - /*OwnsHeaderSearch=*/false,<br>
>> - /*DelayInitialization=*/true);<br>
>> + /*OwnsHeaderSearch=*/false);<br>
>> Preprocessor &PP = *AST->PP;<br>
>> AST->Ctx = new ASTContext(AST->ASTFileLangOpts,<br>
>><br>
>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)<br>
>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Thu May 1 22:43:30 2014<br>
>> @@ -240,11 +240,11 @@ void CompilerInstance::createPreprocesso<br>
>> getLangOpts(),<br>
>> &getTarget());<br>
>> PP = new Preprocessor(&getPreprocessorOpts(),<br>
>> - getDiagnostics(), getLangOpts(), &getTarget(),<br>
>> + getDiagnostics(), getLangOpts(),<br>
>> getSourceManager(), *HeaderInfo, *this, PTHMgr,<br>
>> /*OwnsHeaderSearch=*/true,<br>
>> - /*DelayInitialization=*/false,<br>
>> TUKind);<br>
>> + PP->Initialize(getTarget());<br>
>> // Note that this is different then passing PTHMgr to Preprocessor's<br>
>> ctor.<br>
>> // That argument is used as the IdentifierInfoLookup argument to<br>
>><br>
>> Modified: cfe/trunk/lib/Lex/Preprocessor.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/Preprocessor.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/Preprocessor.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/lib/Lex/Preprocessor.cpp (original)<br>
>> +++ cfe/trunk/lib/Lex/Preprocessor.cpp Thu May 1 22:43:30 2014<br>
>> @@ -56,11 +56,11 @@ ExternalPreprocessorSource::~ExternalPre<br>
>> Preprocessor::Preprocessor(IntrusiveRefCntPtr<PreprocessorOptions><br>
>> PPOpts,<br>
>> DiagnosticsEngine &diags, LangOptions &opts,<br>
>> - const TargetInfo *target, SourceManager &SM,<br>
>> + SourceManager &SM,<br>
>> HeaderSearch &Headers, ModuleLoader<br>
>> &TheModuleLoader,<br>
>> IdentifierInfoLookup *IILookup, bool<br>
>> OwnsHeaders,<br>
>> - bool DelayInitialization, TranslationUnitKind<br>
>> TUKind)<br>
>> - : PPOpts(PPOpts), Diags(&diags), LangOpts(opts), Target(target),<br>
>> + TranslationUnitKind TUKind)<br>
>> + : PPOpts(PPOpts), Diags(&diags), LangOpts(opts), Target(0),<br>
>> FileMgr(Headers.getFileMgr()), SourceMgr(SM), HeaderInfo(Headers),<br>
>> TheModuleLoader(TheModuleLoader), ExternalSource(0),<br>
>> Identifiers(opts, IILookup), IncrementalProcessing(false),<br>
>> @@ -132,11 +132,6 @@ Preprocessor::Preprocessor(IntrusiveRefC<br>
>> Ident___exception_info = Ident___exception_code =<br>
>> Ident___abnormal_termination = 0;<br>
>> Ident_GetExceptionInfo = Ident_GetExceptionCode =<br>
>> Ident_AbnormalTermination = 0;<br>
>> }<br>
>> -<br>
>> - if (!DelayInitialization) {<br>
>> - assert(Target && "Must provide target information for PP<br>
>> initialization");<br>
>> - Initialize(*Target);<br>
>> - }<br>
>> }<br>
>> Preprocessor::~Preprocessor() {<br>
>><br>
>> Modified: cfe/trunk/unittests/Basic/SourceManagerTest.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/SourceManagerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/SourceManagerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/unittests/Basic/SourceManagerTest.cpp (original)<br>
>> +++ cfe/trunk/unittests/Basic/SourceManagerTest.cpp Thu May 1 22:43:30<br>
>> 2014<br>
>> @@ -80,11 +80,11 @@ TEST_F(SourceManagerTest, isBeforeInTran<br>
>> VoidModuleLoader ModLoader;<br>
>> HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr, Diags,<br>
>> LangOpts,<br>
>> &*Target);<br>
>> - Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> Target.getPtr(),<br>
>> + Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> PP.EnterMainSourceFile();<br>
>> std::vector<Token> toks;<br>
>> @@ -195,11 +195,11 @@ TEST_F(SourceManagerTest, getMacroArgExp<br>
>> VoidModuleLoader ModLoader;<br>
>> HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr, Diags,<br>
>> LangOpts,<br>
>> &*Target);<br>
>> - Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> Target.getPtr(),<br>
>> + Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> PP.EnterMainSourceFile();<br>
>> std::vector<Token> toks;<br>
>> @@ -293,11 +293,11 @@ TEST_F(SourceManagerTest, isBeforeInTran<br>
>> VoidModuleLoader ModLoader;<br>
>> HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr, Diags,<br>
>> LangOpts,<br>
>> &*Target);<br>
>> - Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> Target.getPtr(),<br>
>> + Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> std::vector<MacroAction> Macros;<br>
>> PP.addPPCallbacks(new MacroTracker(Macros));<br>
>><br>
>> Modified: cfe/trunk/unittests/Lex/LexerTest.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/LexerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/LexerTest.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/unittests/Lex/LexerTest.cpp (original)<br>
>> +++ cfe/trunk/unittests/Lex/LexerTest.cpp Thu May 1 22:43:30 2014<br>
>> @@ -69,10 +69,10 @@ protected:<br>
>> VoidModuleLoader ModLoader;<br>
>> HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr, Diags,<br>
>> LangOpts,<br>
>> Target.getPtr());<br>
>> - Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> Target.getPtr(),<br>
>> + Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> SourceMgr, HeaderInfo, ModLoader, /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/ false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/ false);<br>
>> + PP.Initialize(*Target);<br>
>> PP.EnterMainSourceFile();<br>
>> std::vector<Token> toks;<br>
>><br>
>> Modified: cfe/trunk/unittests/Lex/PPCallbacksTest.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/unittests/Lex/PPCallbacksTest.cpp (original)<br>
>> +++ cfe/trunk/unittests/Lex/PPCallbacksTest.cpp Thu May 1 22:43:30 2014<br>
>> @@ -173,11 +173,10 @@ protected:<br>
>> IntrusiveRefCntPtr<PreprocessorOptions> PPOpts = new<br>
>> PreprocessorOptions();<br>
>> Preprocessor PP(PPOpts, Diags, LangOpts,<br>
>> - Target.getPtr(),<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> InclusionDirectiveCallbacks* Callbacks = new<br>
>> InclusionDirectiveCallbacks;<br>
>> PP.addPPCallbacks(Callbacks); // Takes ownership.<br>
>> @@ -208,11 +207,10 @@ protected:<br>
>> OpenCLLangOpts, Target.getPtr());<br>
>> Preprocessor PP(new PreprocessorOptions(), Diags, OpenCLLangOpts,<br>
>> - Target.getPtr(),<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> // parser actually sets correct pragma handlers for preprocessor<br>
>> // according to LangOptions, so we init Parser to register opencl<br>
>><br>
>> Modified: cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp?rev=207825&r1=207824&r2=207825&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp?rev=207825&r1=207824&r2=207825&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp<br>
>> (original)<br>
>> +++ cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Thu May<br>
>> 1 22:43:30 2014<br>
>> @@ -97,11 +97,11 @@ TEST_F(PPConditionalDirectiveRecordTest,<br>
>> VoidModuleLoader ModLoader;<br>
>> HeaderSearch HeaderInfo(new HeaderSearchOptions, SourceMgr, Diags,<br>
>> LangOpts,<br>
>> Target.getPtr());<br>
>> - Preprocessor PP(new PreprocessorOptions(), Diags,<br>
>> LangOpts,Target.getPtr(),<br>
>> + Preprocessor PP(new PreprocessorOptions(), Diags, LangOpts,<br>
>> SourceMgr, HeaderInfo, ModLoader,<br>
>> /*IILookup =*/ 0,<br>
>> - /*OwnsHeaderSearch =*/false,<br>
>> - /*DelayInitialization =*/ false);<br>
>> + /*OwnsHeaderSearch =*/false);<br>
>> + PP.Initialize(*Target);<br>
>> PPConditionalDirectiveRecord *<br>
>> PPRec = new PPConditionalDirectiveRecord(SourceMgr);<br>
>> PP.addPPCallbacks(PPRec);<br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
> --<br>
> <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
> the browser experts<br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</blockquote></div><br></div></div>