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

[Leonard Mosescu via Phabricator via llvm-commits]

lemo 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();
----------------
zturner wrote:
> 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?
my main concern is consistency - having multiple flavors of assert can be confusing (one should not have to think too much about the subtleties around the various flavors of "assert")

so my 2c: use lldbassert() in the LLDB codebase


https://reviews.llvm.org/D53002