This is an archive of the discontinued LLVM Phabricator instance.

[LLVM-C] Move DIBuilder Bindings For Temporary MDNodes
ClosedPublic

Authored by CodaFi on May 9 2018, 12:12 AM.

Details

Summary

Move LLVMTemporaryMDNode and LLVMMetadataReplaceAllUsesWith to the C bindings and add LLVMDeleteTemporaryMDNode for deleting non-RAUW'ed temporary nodes.

Diff Detail

Repository
rL LLVM

Event Timeline

CodaFi created this revision.May 9 2018, 12:12 AM
whitequark requested changes to this revision.May 9 2018, 6:24 AM
whitequark added inline comments.
include/llvm-c/DebugInfo.h
851

I think this should be called LLVMDisposeTemporaryMDNode, by analogy with every other destructor.

858

Is this really necessary? If this function remains, it should at least be documented that it is exactly the same as calling LLVMReplaceAllUsesWith and then LLVMDisposeTemporaryMDNode, but I'm not convinced that it has much value.

This revision now requires changes to proceed.May 9 2018, 6:24 AM
CodaFi updated this revision to Diff 146055.May 9 2018, 8:18 PM

llvm::Value::replaceAllUsesWith and llvm::MDNode::replaceAllUsesWith are distinct operations. The former assumes the source and replacement values are the same type, the latter assumes that you're trying to build up a graph of dummy metadata for an incomplete or recursive definition that you will eventually finalize by replacing it with real nodes.

I do agree with the name change for dispose - and it is actually a useful operation to have. We're handing ownership of temporary metadata nodes to the caller and if they don't RAUW they will leak.

CodaFi marked 2 inline comments as done.May 9 2018, 8:19 PM
whitequark accepted this revision.May 10 2018, 7:48 AM

llvm::Value::replaceAllUsesWith and llvm::MDNode::replaceAllUsesWith are distinct operations.

Ah, I haven't realized the MDNode function is different. LGTM.

This revision is now accepted and ready to land.May 10 2018, 7:48 AM
This revision was automatically updated to reflect the committed changes.