[PATCH] D53002: Create a new symbol file plugin for cross-platform PDB symbolization

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:34
+PdbIndex::create(std::unique_ptr<llvm::pdb::PDBFile> file) {
+  assert(file);
+  auto ExpectedDBI = file->getPDBDbiStream();
----------------
lemo wrote:
> zturner wrote:
> > lemo wrote:
> > > lldbassert() ? 
> > > 
> > > (same comment for the rest of assert()s in the CR)
> > `lldbassert` is nice to validate things that you expect should happen but want to log an error if the condition is not held.  `assert` is better when you want to guarantee invariants of your API.  In this case I use `assert` because the precondition of this function is "`file` is non-null".  `lldbassert` wouldn't help here, because the very next line of code is to just dereference the pointer and we'd crash anyway.
> I believe that lldbassert has been fixed (lldb_private::lldb_assert() unconditionally ends with calling abort())
Hmm, that's new.  So it looks to me like the only difference between `assert` and `lldbassert` is that you can't turn `lldbassert` off in release mode.  Do I have that right?  Do we care about that?


https://reviews.llvm.org/D53002