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

[Dmitri Gribenko]

Hi Chandler,

On Fri, Jul 6, 2012 at 1:39 AM, Chandler Carruth <chandlerc at google.com> wrote:
> 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.
> ;]

Thank you for the review!

But regular new (for BumpPtrAllocator) will not compute it correctly
either, will it?

> There is a reason why the allocator didn't provide an array based new before
> -- it is much harder to make correct.

Could you elaborate on this?  How is new[] different from non-array
case w.r.t. alignment?

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

Done in r159833, r159834.

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

OK.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/