On Thu, May 1, 2014 at 8:57 PM, Alp Toker <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > -- > http://www.nuanti.com > the browser experts > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
