[llvm-commits] [llvm] r42051 - in /llvm/trunk: lib/VMCore/Verifier.cpp test/CodeGen/Generic/GC/lower_gcroot.ll test/Verifier/gcread-ptrptr.ll test/Verifier/gcroot-alloca.ll test/Verifier/gcroot-meta.ll test/Verifier/gcroot-ptrptr.ll test/Verifier/gcwrite-ptrptr.ll

[Gordon Henriksen]

On Sep 18, 2007, at 04:49, Duncan Sands wrote:

> Hi Gordon,
>
>> +static bool HasPtrPtrType(Value *Val) {
>
> I think hasPtrPtrType would be more in keeping with the LLVM naming  
> rules (whatever they are).

Do you think so? The convention I had gleaned is that local, private,  
or static (in the C sense) identifiers were usually distinguished by  
InitialCaps.

>> +static Value *StripBitCasts(Value *Val) {
>
> how about using IntrinsicInst::StripPointerCasts instead?

Done. Note that, although there's tantalizing GEP-0 handling in  
StripPointerCasts, it's still not really possible to use gcroot  
without bitcasts:

%class.Object = { %vtable* }
%class.Derived = { %class.Object, ... }

declare void @llvm.gcroot(%class.Object* %root, i8* %meta)

define void @look_ma_no_casts() {
entry:
	%x_addr = alloca %class.Derived*
	%tmp0 = getelementptr %x_addr, i32 0, i32 0  ; Invalid! But nice try...
	call void @llvm.gcroot(%class.Obj* %tmp0, i8* null)
	ret void
}

>> +            "llvm.gcroot parameter #1 must be an alloca (or a  
>> bitcast).", &CI);
>
> "(or a bitcast)" -> "(or a bitcast of one)"

Done.

>> +            "llvm.gcroot parameter #2 must be a constant or  
>> global.", &CI);
>
> how about dropping " or global" since it seems redundant.

I agree. The manual used that phrasing, I think for clarity to those  
not quite so familiar with LLVM's type system. Done.

Thanks Duncan.

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of- 
Mon-20070917/053633.html

— Gordon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070918/cf5aeca0/attachment.html>