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-commits
mailing list