[PATCH] D35908: [YAML] Use {Specific}BumpPtrAllocators for HNodes.

[Juergen Ributzka via llvm-commits]

The SpecificBumpPtrAllocator doesn't have this problem, because it is
already tied to a specific type. Although the convenience and having
consistent API across the BumpPtrAllocators would be nice. Either way, such
a change should be independent of this patch and we should make a separate
patch for the BumpPtrAllocator changes.

On Thu, Jul 27, 2017 at 11:09 AM, Zachary Turner <zturner at google.com> wrote:

> I'm always a little bothered when users of a library have to write a
> placement new, I feel like this kind of thing should be hidden in the
> library as much as possible.
>
> Would it be possible to modify BumpPtrAllocator and
> SpecificBumpPtrAllocator to have an AllocateAndConstruct that wraps the
> logic here so we can get rid of the placement new (which would have
> prevented this typo in the first place)
> On Thu, Jul 27, 2017 at 10:30 AM Juergen Ributzka via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> ributzka marked an inline comment as done.
>> ributzka added inline comments.
>>
>>
>> ================
>> Comment at: llvm/lib/Support/YAMLTraits.cpp:357
>>      }
>> -    return llvm::make_unique<ScalarHNode>(N, KeyStr);
>> +    return new (HNodeAllocator.Allocate<ScalarNode>()) ScalarHNode(N,
>> KeyStr);
>>    } else if (BlockScalarNode *BSN = dyn_cast<BlockScalarNode>(N)) {
>> ----------------
>> zturner wrote:
>> > This is a little confusing.  Why are you calling
>> `Allocate<ScalarNode>()` but then actually constructing a `ScalarHNode`
>> instead of a `ScalarNode`?
>> Oops, that is a typo. Thanks for catching this.
>>
>>
>> https://reviews.llvm.org/D35908
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170728/693e0dca/attachment.html>