[PATCH] D22308: [pdb] Introduce an MsfBuilder class for laying out PDB files

[Zachary Turner via llvm-commits]

The reason I still kept reset is because without it, subsequent calls to
build will generate invalid layouts, which seemed unintuitive and error
prone. I can fix build() to support this, but it's kind of tricky. Even if
someone can create another builder they shouldn't be able to generate a bad
layout.

About initialize(), In earlier revisions this function could fail, so it
needed to return an Error. But I removed the ability for it to fail.
Although now that I think about it, I should probably validate the block
size.


On Wed, Jul 13, 2016 at 3:30 PM Rui Ueyama <ruiu at google.com> wrote:

> ruiu added inline comments.
>
> ================
> Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:33
> @@ +32,3 @@
> +                  bool CanGrow = true);
> +  void reset();
> +
> ----------------
> zturner wrote:
> > ruiu wrote:
> > > What's the use case of `reset()`? You can create as many instances of
> this class as you want, so wouldn't it be easier to make it a non-reusable
> class?
> > `build()` calls `reset()` after it's done to invalidate the entire
> structure so you have to start over.  It would be nice if you could call
> `build()`, make one or two simple changes, then call `build()` again, but
> it's a little bit tricky for technical reasons, so for now I'm just having
> `build()` invalidate it.
> I think what I was trying to say is to move the code from `reset` to the
> ctor if needed and remove the `reset` member function entirely. If a user
> wants to create two or more MSF headers, they can create multiple instances
> of MsfBuilder, so `reset` does not seem necessary.
>
> ================
> Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:26-27
> @@ +25,4 @@
> +
> +void MsfBuilder::initialize(uint32_t PageSize, uint32_t MinPageCount,
> +                            bool CanGrow) {
> +  assert(!IsInitialized && "MsfBuilder is already initialized!");
> ----------------
> I think this is a bit error-prone. Why don't you merge this with the ctor?
>
>
> http://reviews.llvm.org/D22308
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160713/aa221a8c/attachment.html>