labath added a comment.
The problem I have is that PDB is not following the lldb convention of doing
the symbol file search inside a SymbolVendor class (which should really be
called a SymbolFinder), but instead they implement their own little symbol
vendor privately. There are reasons which make switching that not trivial, but
they're not important in the context of this patch, which seems fine to me, as
any reasonable future symbol vendor should also support the standard windows
search mechanism. So overall, I do not see a reason to not accept this patch.
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:110
+
+static std::string findPdbFile(const llvm::StringRef exe_path, const
llvm::StringRef pdb_file, llvm::file_magic &magic) {
+ auto ec = llvm::identify_magic(pdb_file, magic);
----------------
I find the interface of this function odd. First, the `const`s on the StringRef
argument are useless and should be removed. Secondly, the by-ref return of the
`magic` argument is something that would be nice to avoid. It looks like that
could easily be done here by just checking whether the file exists and doing
the identify_magic check in the caller (if you want an existing-but-not-pdb
file to abort the search), or by checking the signature in this function (if
you want to skip past non-pdb files). Also, this patch could use
clang-formatting as this line is over the column limit.
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:111
+static std::string findPdbFile(const llvm::StringRef exe_path, const
llvm::StringRef pdb_file, llvm::file_magic &magic) {
+ auto ec = llvm::identify_magic(pdb_file, magic);
+ if (!ec)
----------------
Llvm policy is to use `auto` "if and only if it makes the code more readable"
<http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>.
Whether that's the case here is somewhat subjective, but I'd say that none of
the uses of auto in this patch are helping readability, as all the types used
in this patch are short enough and spelling them out saves the reader from
guessing whether `ec` really is a `std::error_code`, etc.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60962/new/
https://reviews.llvm.org/D60962
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
