[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

[Mikhail Senkov via Phabricator via lldb-commits]

zloyrobot marked 2 inline comments as done.
zloyrobot added inline comments.


================
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);
----------------
labath wrote:
> zloyrobot wrote:
> > labath wrote:
> > > 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.
> > I want to skip past non-pdb files. Am I understand correctly that you suggest me to get rid of file_magic parameter and call identify_magic (open and read pdb file) additional time (in caller)? 
> In that case, what you could do is get rid of the `magic` as an argument, and change this function to return a valid result, only if you found the file *and* that files magic number is correct. (note that this is not what the current implementation does)
got it, thanks


================
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)
----------------
labath wrote:
> zloyrobot wrote:
> > labath wrote:
> > > 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.
> > Please note that I moved  ```auto ec = ...```  from original Turner's code 
> yeah, I know, but that doesn't make it right. :) In fact, based on a random sample, I would guess that about 90% of uses of `auto ec` in llvm is @zturner's code. :P
> 
> TBH, the `ec` is no the part I'm having problems with the most. If it was just that, I wouldn't mention it, but the fact that you're using it all over the place tells me you weren't aware of llvm's policy regarding `auto` (which explicitly states "don't 'almost always use auto'"), and I figured it's best to mention that early on. 
Thanks for the clarifications, I updated the path


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60962