[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:769-774
+FileSpec::Style DWARFUnit::GetPathStyle() {
+  if (!m_comp_dir)
+    ComputeCompDirAndGuessPathStyle();
+  return m_comp_dir->GetPathStyle();
+}
+
----------------
Remove?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:777
+  if (!m_comp_dir)
+    ComputeCompDirAndGuessPathStyle();
+  return *m_comp_dir;
----------------
Just move contents of ComputeCompDirAndGuessPathStyle() to this function now that we don't have GetPathStyle?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:818
+
+void DWARFUnit::ComputeCompDirAndGuessPathStyle() {
+  m_comp_dir = FileSpec();
----------------
Move contents to DWARFUnit::GetCompilationDirectory?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:173
+  const lldb_private::FileSpec &GetCompilationDirectory();
+  lldb_private::FileSpec::Style GetPathStyle();
+
----------------
Is GetPathStyle() needed? Seems like we should be able to grab the style from:

```
auto style = unit->GetCompilationDirectory().GetPathStyle();
```



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:256
 
+  void ComputeCompDirAndGuessPathStyle();
+
----------------
If we remove GetPathStyle above, should this be removed and just put into the code for GetCompilationDirectory() now that we don't have two accessors?


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:757
           if (cu_die) {
-            FileSpec cu_file_spec(cu_die.GetName());
+            FileSpec cu_file_spec(cu_die.GetName(), dwarf_cu->GetPathStyle());
             if (cu_file_spec) {
----------------
Not sure how we would resolve this one. What if dwarf_cu doesn't have a DW_AT_comp_dir? Maybe we add a FileSpec constructor that takes a string and a FileSpec &relative_dir so this would would just become:

```
FileSpec cu_fs(cu_die.GetName(), dwarf_cu->GetCompilationDirectory());
```

The proto would be something like:
```
explicit FileSpec::FileSpec(llvm::StringRef path, const FileSpec &relative_root);
```

Then the if statement below goes away.


================
Comment at: source/Utility/FileSpec.cpp:560
 
+void FileSpec::MakeAbsolute(const FileSpec &dir) {
+  if (IsRelative())
----------------
Should this be "bool FileSpec::ResolveRelativePath(const FileSpec &dir)"? Returns true if this path was relative and it was resolved, and false otherwise?


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

https://reviews.llvm.org/D56543