[llvm-commits] [llvm] r159789 - /llvm/trunk/include/llvm/Support/Allocator.h

[Chandler Carruth]

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.
;]

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.

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.

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.


On Fri, Jul 6, 2012 at 12:49 AM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Dmitri,
>
> > --- llvm/trunk/include/llvm/Support/Allocator.h (original)
> > +++ llvm/trunk/include/llvm/Support/Allocator.h Thu Jul  5 19:25:39 2012
> > @@ -239,4 +239,21 @@
> >
> >   inline void operator delete(void *, llvm::BumpPtrAllocator &) {}
> >
> > +inline void *operator new[](size_t Size, llvm::BumpPtrAllocator
> &Allocator) {
> > +  struct S {
> > +    char c;
> > +    union {
> > +      double D;
> > +      long double LD;
> > +      long long L;
> > +      void *P;
> > +    } x;
> > +  };
>
> is this ugly struct really needed?  If so, maybe it should be placed in
> its own
> header file somewhere and shared with code like SmallVector.h that does
> the same
> trick.
>
> Ciao, Duncan.
>
> > +  return Allocator.Allocate(Size,
> std::min((size_t)llvm::NextPowerOf2(Size),
> > +                                           offsetof(S, x)));
> > +}
> > +
> > +inline void operator delete[](void *Ptr, llvm::BumpPtrAllocator &C,
> size_t) {
> > +}
> > +
> >   #endif // LLVM_SUPPORT_ALLOCATOR_H
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120706/312a8c14/attachment.html>