On Tue, Feb 23, 2016 at 12:46 PM, Nico Weber <tha...@chromium.org> wrote:
> On Tue, Feb 23, 2016 at 2:44 PM, David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> >> >> On Tue, Feb 23, 2016 at 11:30 AM, Nico Weber via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: nico >>> Date: Tue Feb 23 13:30:43 2016 >>> New Revision: 261674 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=261674&view=rev >>> Log: >>> Rename Action::begin() to Action::input_begin(). >>> >>> Also introduce inputs() that reutnrs an llvm::iterator_range. >>> Iterating over A->inputs() is much less mysterious than >>> iterating over *A. No intended behavior change. >>> >> >> Seems like a strange change - is there ambiguity of what an Action is a >> collection of? >> > > Action isn't primarily a collection, but an action (a list of inputs, but > also a kind, an output type, etc) :-) > The analogy to llvm::Function, llvm::BasicBlock, etc, still seem somewhat apt (a Function isn't, in some sense, primarily a collection of basic blocks - it's a global value with a name and parameters, etc). > I found this code pretty confusing, hence I renamed it. > > (I do have a local change currently that gives it a second iterable thing, > but this seemed like a good change independent of my local change.) > Fair enough - just figured I'd point out there's a fair bit of precedent for thing-is-a-collection across LLVM, in case that provides a diferent perspective. - Dave > > >> (does it expose other collections (perhaps its outputs?) - or only its >> inputs? ) >> >> Lots of things in LLVM/Clang are containers (functions are containers of >> basic blocks, basic blocks are containers of instructions, etc) so this >> doesn't seem too strange to me... perhaps there's something different about >> this situation/API? >> >> >>> >>> Modified: >>> cfe/trunk/include/clang/Driver/Action.h >>> cfe/trunk/lib/Driver/Compilation.cpp >>> cfe/trunk/lib/Driver/Driver.cpp >>> cfe/trunk/lib/Driver/Tools.cpp >>> cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp >>> cfe/trunk/lib/Tooling/CompilationDatabase.cpp >>> >>> Modified: cfe/trunk/include/clang/Driver/Action.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Action.h?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Driver/Action.h (original) >>> +++ cfe/trunk/include/clang/Driver/Action.h Tue Feb 23 13:30:43 2016 >>> @@ -41,8 +41,10 @@ namespace driver { >>> class Action { >>> public: >>> typedef ActionList::size_type size_type; >>> - typedef ActionList::iterator iterator; >>> - typedef ActionList::const_iterator const_iterator; >>> + typedef ActionList::iterator input_iterator; >>> + typedef ActionList::const_iterator input_const_iterator; >>> + typedef llvm::iterator_range<input_iterator> input_range; >>> + typedef llvm::iterator_range<input_const_iterator> input_const_range; >>> >>> enum ActionClass { >>> InputClass = 0, >>> @@ -98,10 +100,14 @@ public: >>> >>> size_type size() const { return Inputs.size(); } >>> >>> - iterator begin() { return Inputs.begin(); } >>> - iterator end() { return Inputs.end(); } >>> - const_iterator begin() const { return Inputs.begin(); } >>> - const_iterator end() const { return Inputs.end(); } >>> + input_iterator input_begin() { return Inputs.begin(); } >>> + input_iterator input_end() { return Inputs.end(); } >>> + input_range inputs() { return input_range(input_begin(), >>> input_end()); } >>> + input_const_iterator input_begin() const { return Inputs.begin(); } >>> + input_const_iterator input_end() const { return Inputs.end(); } >>> + input_const_range inputs() const { >>> + return input_const_range(input_begin(), input_end()); >>> + } >>> }; >>> >>> class InputAction : public Action { >>> >>> Modified: cfe/trunk/lib/Driver/Compilation.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Compilation.cpp?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Driver/Compilation.cpp (original) >>> +++ cfe/trunk/lib/Driver/Compilation.cpp Tue Feb 23 13:30:43 2016 >>> @@ -176,8 +176,8 @@ static bool ActionFailed(const Action *A >>> if (A == &(CI->second->getSource())) >>> return true; >>> >>> - for (Action::const_iterator AI = A->begin(), AE = A->end(); AI != AE; >>> ++AI) >>> - if (ActionFailed(*AI, FailingCommands)) >>> + for (const Action *AI : A->inputs()) >>> + if (ActionFailed(AI, FailingCommands)) >>> return true; >>> >>> return false; >>> >>> Modified: cfe/trunk/lib/Driver/Driver.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Driver/Driver.cpp (original) >>> +++ cfe/trunk/lib/Driver/Driver.cpp Tue Feb 23 13:30:43 2016 >>> @@ -948,15 +948,15 @@ static unsigned PrintActions1(const Comp >>> os << "\"" << IA->getInputArg().getValue() << "\""; >>> } else if (BindArchAction *BIA = dyn_cast<BindArchAction>(A)) { >>> os << '"' << BIA->getArchName() << '"' << ", {" >>> - << PrintActions1(C, *BIA->begin(), Ids) << "}"; >>> + << PrintActions1(C, *BIA->input_begin(), Ids) << "}"; >>> } else if (CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) { >>> os << '"' >>> << (CDA->getGpuArchName() ? CDA->getGpuArchName() : "(multiple >>> archs)") >>> - << '"' << ", {" << PrintActions1(C, *CDA->begin(), Ids) << "}"; >>> + << '"' << ", {" << PrintActions1(C, *CDA->input_begin(), Ids) << >>> "}"; >>> } else { >>> const ActionList *AL; >>> if (CudaHostAction *CHA = dyn_cast<CudaHostAction>(A)) { >>> - os << "{" << PrintActions1(C, *CHA->begin(), Ids) << "}" >>> + os << "{" << PrintActions1(C, *CHA->input_begin(), Ids) << "}" >>> << ", gpu binaries "; >>> AL = &CHA->getDeviceActions(); >>> } else >>> @@ -996,7 +996,7 @@ static bool ContainsCompileOrAssembleAct >>> isa<AssembleJobAction>(A)) >>> return true; >>> >>> - for (const Action *Input : *A) >>> + for (const Action *Input : A->inputs()) >>> if (ContainsCompileOrAssembleAction(Input)) >>> return true; >>> >>> @@ -1747,8 +1747,8 @@ static const Tool *selectToolForJob(Comp >>> // Compile job may be wrapped in CudaHostAction, extract it if >>> // that's the case and update CollapsedCHA if we combine phases. >>> CudaHostAction *CHA = >>> dyn_cast<CudaHostAction>(*BackendInputs->begin()); >>> - JobAction *CompileJA = >>> - cast<CompileJobAction>(CHA ? *CHA->begin() : >>> *BackendInputs->begin()); >>> + JobAction *CompileJA = cast<CompileJobAction>( >>> + CHA ? *CHA->input_begin() : *BackendInputs->begin()); >>> assert(CompileJA && "Backend job is not preceeded by compile job."); >>> const Tool *Compiler = TC->SelectTool(*CompileJA); >>> if (!Compiler) >>> @@ -1770,7 +1770,7 @@ static const Tool *selectToolForJob(Comp >>> // that's the case and update CollapsedCHA if we combine phases. >>> CudaHostAction *CHA = dyn_cast<CudaHostAction>(*Inputs->begin()); >>> JobAction *CompileJA = >>> - cast<CompileJobAction>(CHA ? *CHA->begin() : *Inputs->begin()); >>> + cast<CompileJobAction>(CHA ? *CHA->input_begin() : >>> *Inputs->begin()); >>> assert(CompileJA && "Backend job is not preceeded by compile job."); >>> const Tool *Compiler = TC->SelectTool(*CompileJA); >>> if (!Compiler) >>> @@ -1841,7 +1841,7 @@ InputInfo Driver::BuildJobsForActionNoCa >>> } >>> // Override current action with a real host compile action and >>> continue >>> // processing it. >>> - A = *CHA->begin(); >>> + A = *CHA->input_begin(); >>> } >>> >>> if (const InputAction *IA = dyn_cast<InputAction>(A)) { >>> @@ -1867,7 +1867,7 @@ InputInfo Driver::BuildJobsForActionNoCa >>> else >>> TC = &C.getDefaultToolChain(); >>> >>> - return BuildJobsForAction(C, *BAA->begin(), TC, ArchName, >>> AtTopLevel, >>> + return BuildJobsForAction(C, *BAA->input_begin(), TC, ArchName, >>> AtTopLevel, >>> MultipleArchs, LinkingOutput, >>> CachedResults); >>> } >>> >>> @@ -1875,11 +1875,11 @@ InputInfo Driver::BuildJobsForActionNoCa >>> // Initial processing of CudaDeviceAction carries host params. >>> // Call BuildJobsForAction() again, now with correct device >>> parameters. >>> InputInfo II = BuildJobsForAction( >>> - C, *CDA->begin(), C.getCudaDeviceToolChain(), >>> CDA->getGpuArchName(), >>> - CDA->isAtTopLevel(), /*MultipleArchs*/ true, LinkingOutput, >>> - CachedResults); >>> - // Currently II's Action is *CDA->begin(). Set it to CDA instead, >>> so that >>> - // one can retrieve II's GPU arch. >>> + C, *CDA->input_begin(), C.getCudaDeviceToolChain(), >>> + CDA->getGpuArchName(), CDA->isAtTopLevel(), >>> /*MultipleArchs=*/true, >>> + LinkingOutput, CachedResults); >>> + // Currently II's Action is *CDA->input_begin(). Set it to CDA >>> instead, so >>> + // that one can retrieve II's GPU arch. >>> II.setAction(A); >>> return II; >>> } >>> @@ -2364,7 +2364,8 @@ const ToolChain &Driver::getToolChain(co >>> >>> bool Driver::ShouldUseClangCompiler(const JobAction &JA) const { >>> // Say "no" if there is not exactly one input of a type clang >>> understands. >>> - if (JA.size() != 1 || >>> !types::isAcceptedByClang((*JA.begin())->getType())) >>> + if (JA.size() != 1 || >>> + !types::isAcceptedByClang((*JA.input_begin())->getType())) >>> return false; >>> >>> // And say "no" if this is not a kind of action clang understands. >>> >>> Modified: cfe/trunk/lib/Driver/Tools.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Driver/Tools.cpp (original) >>> +++ cfe/trunk/lib/Driver/Tools.cpp Tue Feb 23 13:30:43 2016 >>> @@ -2471,8 +2471,8 @@ static bool ContainsCompileAction(const >>> if (isa<CompileJobAction>(A) || isa<BackendJobAction>(A)) >>> return true; >>> >>> - for (const auto &Act : *A) >>> - if (ContainsCompileAction(Act)) >>> + for (const auto &AI : A->inputs()) >>> + if (ContainsCompileAction(AI)) >>> return true; >>> >>> return false; >>> >>> Modified: cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp (original) >>> +++ cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp Tue Feb >>> 23 13:30:43 2016 >>> @@ -70,7 +70,7 @@ clang::createInvocationFromCommandLine(A >>> for (auto &A : C->getActions()){ >>> // On MacOSX real actions may end up being wrapped in >>> BindArchAction >>> if (isa<driver::BindArchAction>(A)) >>> - A = *A->begin(); >>> + A = *A->input_begin(); >>> if (isa<driver::CudaDeviceAction>(A)) { >>> CudaCompilation = true; >>> break; >>> >>> Modified: cfe/trunk/lib/Tooling/CompilationDatabase.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/CompilationDatabase.cpp?rev=261674&r1=261673&r2=261674&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Tooling/CompilationDatabase.cpp (original) >>> +++ cfe/trunk/lib/Tooling/CompilationDatabase.cpp Tue Feb 23 13:30:43 >>> 2016 >>> @@ -139,9 +139,8 @@ private: >>> ; >>> } >>> >>> - for (driver::ActionList::const_iterator I = A->begin(), E = >>> A->end(); >>> - I != E; ++I) >>> - runImpl(*I, CollectChildren); >>> + for (const driver::Action *AI : A->inputs()) >>> + runImpl(AI, CollectChildren); >>> } >>> }; >>> >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
