[PATCH] D28091: Add a class to create a tar archive file.

[Rui Ueyama via llvm-commits]

On Fri, Jan 6, 2017 at 7:22 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Rui Ueyama via Phabricator <reviews at reviews.llvm.org> writes:
> > +static std::string formatPax(StringRef Key, const Twine &Val) {
> > +  int Len = Key.size() + Val.str().size() + 3; // +3 for " ", "=" and
> > "\n"
>
> Nit: Save the temporary str() and you can reused them in the function.
>

I could, but because the data this function will handle is small, I think
we don't need to optimize it.


> > +// Headers in tar files must be aligned to 512 byte boundaries.
> > +// This function writes null bytes so that the file is a multiple
> > +// of 512 bytes.
> > +static void pad(raw_ostream &OS) {
> > +  uint64_t Pos = OS.tell();
> > +  if (Pos % BlockSize == 0)
> > +    return;
> > +  OS << std::string(BlockSize - Pos % BlockSize, '\0');
>
> Nit: Don't allocate a string for this.
>

Changed to use OS.seek().


> > +// We want to use '/' as a path separator even on Windows.
> > +// This function canonicalizes a given path.
> > +static std::string canonicalize(StringRef S) {
>
> this is lld::convertToUnixPathSeparator.
>

Yes, but we cannot use LLD's function from LLVM.


>
> This is a lot simpler than what I was expecting when I first looked at
> tar. I agree that while the format is ugly, it is far more common and
> the command line is more user friendly.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170106/90c4401c/attachment.html>