[cfe-dev] Default arguments considered harmful?
Richard Smith
richard at metafoo.co.uk
Thu May 1 21:26:29 PDT 2014
On Thu, May 1, 2014 at 9:02 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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);
>
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.
> 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
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140501/5ba5231c/attachment.html>
More information about the cfe-dev
mailing list