labath marked 5 inline comments as done.
labath added inline comments.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:173
+ const lldb_private::FileSpec &GetCompilationDirectory();
+ lldb_private::FileSpec::Style GetPathStyle();
+
----------------
clayborg wrote:
> Is GetPathStyle() needed? Seems like we should be able to grab the style from:
>
> ```
> auto style = unit->GetCompilationDirectory().GetPathStyle();
> ```
>
I'd like to avoid that for two reasons:
- I think this documents the intent better. Logically, the "path style" is a
property of the compile unit and the fact that the compilation directory (and
everything else) uses this path style is a consequence of that. The fact that
we actually get the path style from the compilation directory is an
implementation detail that users should not care about and may in fact change
in the future e.g. if DWARF develops an ability to explicitly signal this
information.
- I'm not thrilled by the fact that we can have an empty (invalid) FileSpec,
whose path style still has some significance (this happens if the CU does not
have a comp_dir attribute, and we guess the style from the. In fact I even
considered getting rid of that and having an explicit path style member). In
the end I decided it's not too bad if it's internal to DWARFCompileUnit, but I
would not want to expose this quirk to the outside world. So if anything, I'd
actually go for making the path style a separate member.
================
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) {
----------------
clayborg wrote:
> 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.
That would work if we say that we do support having empty/invalid FileSpec
objects with certain PathStyle. However, as I said above, I am not entirely
thrilled by that. FWIW, I don't think having this if is bad, as it makes it
clear that how you're intending to handle the case when the CU has no name.
================
Comment at: source/Utility/FileSpec.cpp:560
+void FileSpec::MakeAbsolute(const FileSpec &dir) {
+ if (IsRelative())
----------------
clayborg wrote:
> Should this be "bool FileSpec::ResolveRelativePath(const FileSpec &dir)"?
> Returns true if this path was relative and it was resolved, and false
> otherwise?
I called this `MakeAbsolute` because that's how the equivalent llvm function is
called (`llvm::sys::fs::make_absolute`). I think it would be good to stay
consistent here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56543/new/
https://reviews.llvm.org/D56543
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
