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

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

> On 2016-Apr-22, at 08:52, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>>> 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.

Umm, why did I think I had to change this call site?

>>> +
>>> 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.

I figured it out and committed in r267271.  Thanks for the review!