[llvm-commits] [llvm] r166607 - in /llvm/trunk: include/llvm/DataLayout.h lib/Analysis/ConstantFolding.cpp lib/Analysis/InlineCost.cpp lib/Analysis/InstructionSimplify.cpp lib/CodeGen/AsmPrinter/AsmPrinter.cpp lib/ExecutionEngine/ExecutionEngine.

[Villmow, Micah]

>> And this just doesn't make sense... Why do you allow non-pointer types
>> as arguments?
> [Villmow, Micah] There were lots of places in the code base where there was not a guarantee that the type being evaluated for the pointer size was a pointer type. In some places it was a vector of pointers, in others the value of 'void' would show up or a label. In these cases, the size of the default pointer is all that is required, so instead of complicating the caller, I moved the logic into the callee to simplify things.

If this function is intended to handle cases where there is no
guarantee that we have a pointer type, because that is the dominant
pattern, then it should have been committed in the same commit that
updates these "lots of places" to use it. The uses I see in this
commit would all be substantially simpler with the definition and
semantics I suggested.

I also can't really figure out in what case you would actually not
know whether or not the type you have is a pointer type, and yet want
a pointer size which is in some way "based" on it... It's just really
unclear what the use case for this could possibly be. On the other
hand, having a routine that assumes (and checks) its argument is a
pointer-like type (either a pointer or a vector of pointers) and
extracts the relevant pointer size from that, seems quite useful.


I suggest you take one of two approaches:

1) Fix the code you have touched here to be really simple by making
the utility function you have added suit their needs, or

2) Revert this patch, and propose a new patch that actually updates
the callers which need the somewhat strange semantics you are
proposing for getPointerTypeSizeInBits along with that function's
implementation.

That way we can review the utility routine and understand the use
cases for it. Currently, I have to assume that the callers you have
updated in this patch are either the only users or at least
representative of the common usage pattern.

Will do, the new patch is attached, I'll go with the first approach and then if I run back into the locations where I was having trouble, I'll fix them up to use different API's.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 001c-data-layout-cleanup.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121024/586dd944/attachment.txt>