Default arguments considered harmful?

David Tweed david.tweed at arm.com
Fri May 2 01:16:59 PDT 2014


I wonder if, with C++11 named initialization syntax for PODs, something
could be done so that in cases like this a (function specific, I guess)
"optional options object" could be used. (This would avoid one of the big
problems with C++ optional arguments, which is that if one towards the end
needs to be set to a non-default value all the preceding options need
setting.) That would certainly make things a lot easier to read in cases
like these.

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu
[mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Alp Toker
Sent: 02 May 2014 04:58
To: cfe-commits at cs.uiuc.edu; clang-dev Developers
Subject: Default arguments considered harmful?

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


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=2
07825&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=2
07825&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/SourceManagerT
est.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?re
v=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/PPConditionalDir
ectiveRecordTest.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