[PATCH] D96678: [llvm-dwp] Join dwo paths correctly when DWOPath is absolute

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/tools/llvm-dwp/X86/absolute_paths.test:4
+; RUN: llc %s --split-dwarf-file=%t/test.dwo --split-dwarf-output=%t/test.dwo --filetype=obj -o %t/test.o
+; RUN: llvm-dwp -e %t/test.o -o %t/test.dwp
+
----------------
Should this have some CHECKs for certain behavior (dumping the resulting dwp and checking things, for instance)

"does something other than crashing/failing" is a pretty low bar for a test, doesn't especially check that this behaves in a way we would want it to.


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:529
     if (!DWOCompDir.empty()) {
-      SmallString<16> DWOPath;
-      sys::path::append(DWOPath, DWOCompDir, DWOName);
+      SmallString<16> DWOPath{std::move(DWOName)};
+      sys::fs::make_absolute(DWOCompDir, DWOPath);
----------------
nagisa wrote:
> dblaikie wrote:
> > Generally prefer `=` init syntax over `{}` or `()` (this avoids any invocation of explicit conversions, for instance - making the code a bit simpler/easier to understand)
> The `=` fails to build, because the conversion is necessary (`DWOName` is a `std::string`). Is there a better way to get this conversion happen?
Ah, no, that's fine then. I'd /probably/ lean towards using () rather than {} then, because there's been no wide-scale stylistic tendency towards {} for ctors in LLVM, but not a huge deal.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96678/new/

https://reviews.llvm.org/D96678