[PATCH] D38149: Fix off-by-one error in TarWriter.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/lib/Support/TarWriter.cpp:128
+// Otherwise, returns false.
+static bool splitUstar(StringRef Path, std::string &Prefix, std::string &Name) {
+  if (Path.size() < sizeof(UstarHeader::Name)) {
----------------
zturner wrote:
> I don't think we need `std::string` here.  Can't we just use `StringRef` again?  Also, how about just returning `Optional<std::pair<StringRef, StringRef>>`?
I actually tried to use `Optional<std::pair<StringRef, StringRef>>` but it looked like it is a bit too complicated type for this little helper function.


================
Comment at: llvm/lib/Support/TarWriter.cpp:134
+
   size_t Sep = Path.rfind('/', sizeof(UstarHeader::Prefix) + 1);
   if (Sep == StringRef::npos)
----------------
zturner wrote:
> What about this Path?
> 
> ```
> <total of 95 a's>/aaaaa/bb
> ```
> 
> in this case we have 95 + 1 + 5 = 101 characters, followed by a `/`, followed by `bb`.
> 
> Does this path conform to Ustar header?  You can split it as `(<total of 95 a's>, "aaaaa/bb")` but then you have a `/` in the name.  Is this ok?
That's OK. Updated the comment a bit.


https://reviews.llvm.org/D38149