[LLVMdev] [cfe-dev] Default arguments considered harmful?
Alp Toker
alp at nuanti.com
Thu May 1 21:37:30 PDT 2014
On 02/05/2014 05:26, Richard Smith wrote:
> On Thu, May 1, 2014 at 9:02 PM, David Blaikie <dblaikie at gmail.com
> <mailto:dblaikie at gmail.com>> wrote:
>
> On Thu, May 1, 2014 at 8:57 PM, Alp Toker <alp at nuanti.com
> <mailto: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.
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 at cs.uiuc.edu <mailto: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 <mailto: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 <mailto:cfe-dev at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
--
http://www.nuanti.com
the browser experts
More information about the llvm-dev
mailing list