Not only is it ugly, it isn't correct either. For example, I suspect it will compute the incorrect alignment for a struct containing an AVX 256-bit entity. And I see why the tests didn't catch this bug: there are no tests. ;]<div>
<br></div><div>This simply doesn't work because C++98 does not provide any realistic mechanism to guess at what is "suitably aligned". C++11 does, but we can't portably rely on this.</div><div><br></div>
<div>There is a reason why the allocator didn't provide an array based new before -- it is much harder to make correct. Please remove this, and use the array based Allocate functions directly which accept a template parameter to inform the code of the type being allocated, and compute the appropriate alignment requirements from that type.</div>
<div><br></div><div>Sorry for the negative feedback here post-commit, but you mailed this patch to llvm-commits buried inside of a long review thread from cfe-commits. Also, Doug OK-ed the clang commit, but I'm not sure he really looked at this part of the patch carefully. In general and for future reference, please wait for review to take place on the mailing list you send the patch to -- many people on llvm-commits don't even read cfe-commits.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 6, 2012 at 12:49 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Dmitri,<br>
<div class="im"><br>
> --- llvm/trunk/include/llvm/Support/Allocator.h (original)<br>
> +++ llvm/trunk/include/llvm/Support/Allocator.h Thu Jul  5 19:25:39 2012<br>
> @@ -239,4 +239,21 @@<br>
><br>
>   inline void operator delete(void *, llvm::BumpPtrAllocator &) {}<br>
><br>
> +inline void *operator new[](size_t Size, llvm::BumpPtrAllocator &Allocator) {<br>
> +  struct S {<br>
> +    char c;<br>
> +    union {<br>
> +      double D;<br>
> +      long double LD;<br>
> +      long long L;<br>
> +      void *P;<br>
> +    } x;<br>
> +  };<br>
<br>
</div>is this ugly struct really needed?  If so, maybe it should be placed in its own<br>
header file somewhere and shared with code like SmallVector.h that does the same<br>
trick.<br>
<br>
Ciao, Duncan.<br>
<div class="HOEnZb"><div class="h5"><br>
> +  return Allocator.Allocate(Size, std::min((size_t)llvm::NextPowerOf2(Size),<br>
> +                                           offsetof(S, x)));<br>
> +}<br>
> +<br>
> +inline void operator delete[](void *Ptr, llvm::BumpPtrAllocator &C, size_t) {<br>
> +}<br>
> +<br>
>   #endif // LLVM_SUPPORT_ALLOCATOR_H<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>