[llvm] r267099 - ValueMapper/Enumerator: Clean up code in post-order traversals, NFC

[Duncan P. N. Exon Smith via llvm-commits]

>> project/llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp?rev=267099&r1=267098&r2=267099&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp (original)
>> +++ llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp Thu Apr 21 21:33:06 2016
>> @@ -569,36 +569,40 @@ void ValueEnumerator::dropFunctionFromMe
>> void ValueEnumerator::EnumerateMetadata(unsigned F, const Metadata *MD) {
>>  // Start by enumerating MD, and then work through its transitive operands in
>>  // post-order.  This requires a depth-first search.
>> -  SmallVector<std::pair<const MDNode *, const MDOperand *>, 32> Worklist;
>> -  enumerateMetadataImpl(F, MD, Worklist);
>> +  SmallVector<std::pair<const MDNode *, MDNode::op_iterator>, 32> Worklist;
>> +  if (const MDNode *N = enumerateMetadataImpl(F, MD))
>> +    Worklist.push_back(std::make_pair(N, N->op_begin()));

^ Note this.

>> +
>>  while (!Worklist.empty()) {
>>    const MDNode *N = Worklist.back().first;
>> -    const MDOperand *&Op = Worklist.back().second; // Be careful of lifetime...
>> +    MDNode::op_iterator &I = Worklist.back().second;
>> 
>>    // Enumerate operands until the worklist changes.  We need to traverse new
>>    // nodes before visiting the rest of N's operands.
>> -    bool DidWorklistChange = false;
>> -    for (const MDOperand *E = N->op_end(); Op != E;)
>> -      if (enumerateMetadataImpl(F, *Op++, Worklist)) {
>> -        DidWorklistChange = true;
>> -        break;
>> -      }
>> -    if (DidWorklistChange)
>> +    if (const MDNode *Op = enumerateMetadataOperands(F, I, N->op_end())) {
>> +      Worklist.push_back(std::make_pair(Op, Op->op_begin()));
> 
> 
> Now that you have nicely refactored it, you almost have a std algorithm at hand. 
> I'd kill enumerateMetadataOperands (there is no other use) and avoid the side effect increment on I (which is not obvious from the call site).
> 
> Something like this:
> 
>    // Enumerate operands until the worklist changes.  We need to traverse new
>    // nodes before visiting the rest of N's operands.
>    I = llvm::find_if(
>        MDNode::op_range(I, N->op_end()),
>        [&](const MDOperand &Op) { return enumerateMetadataImpl(F, Op); });
>    if (I != N->op_end()) {
>      // We find a new operand to process, queue it and continue
>      const MDNode *MD = dyn_cast<MDNode>(*I);
>      Worklist.push_back(std::make_pair(MD, MD->op_begin()));
>      continue;
>    }

I tried this just now (using std::find_if, which fits even better than
llvm::find_if).  It looked great, until I realized I had to duplicate
the boiler-plate check and push_back insertion up above to initialize
the worklist.  I could add a helper for the push, but I don't think
it's strictly better than what's already there.  I think I prefer it
as is.