[lldb-dev] LLDB Evolution - Final Form

[Zachary Turner via lldb-dev]

StringRef has `withNullAsEmpty` which I added a few days ago.  It will
return an empty StringRef.  seems to me that should solve most of those
kinds of problems.

On Tue, Sep 20, 2016 at 2:31 PM Greg Clayton <gclayton at apple.com> wrote:

> We should avoid crashing if there is a reasonable work around when the
> input is bad. StringRef with NULL is easy, just put NULL and zero length
> and don't crash. Just because it is documented, doesn't mean it needs to
> stay that way, but I am not going to fight that battle.
>
> We should make every effort to not crash if we can. If it is way too
> difficult, document the issue and make sure clients know that this is the
> way things are. StringRef does this and we accept it. Doesn't mean it won't
> crash us. I just hate seeing the crash logs where we have:
>
> StringRef s(die.GetName());
>
> It shouldn't crash IMHO, but we know it does and we now code around it.
> Yes, we put in code like:
>
> StringRef s;
> const char *cstr = die.GetName();
> if (cstr)
>     s = cstr;
>
> Is this nice code? I am glad it makes it simple for the LLVM side, but I
> would rather write:
>
> StringRef s(die.GetName());
>
> Maybe I will subclass llvm::StringRef as lldb::StringRef and override the
> constructor.
>
>
> > On Sep 20, 2016, at 2:24 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Well, but StringRef for example is well documented.  So it seems to me
> like there's an example of a perfectly used assert.  It's documented that
> you can't use null, and if you do it asserts.  Just like strlen.
> >
> > The issue I have with "you can't ever assert" is that it brings it into
> an absolute when it really shouldn't be.  We already agreed (I think) that
> certain things that are well documented can assert.  But when we talk in
> absolutes, it tends to sway people that they should always do that thing,
> even when it's not the most appropriate solution.  And I think some of that
> shows in the LLDB codebase where you've got hugely complicated logic that
> is very hard to follow, reason about, or test, because no assumptions are
> ever made about any of the inputs.  Even when they are internal inputs that
> are entirely controlled by us.
> >
> > On Tue, Sep 20, 2016 at 2:19 PM Greg Clayton <gclayton at apple.com> wrote:
> > Again, strlen is a stupid example as it is well documented. All of llvm
> and clang are not.
> > > On Sep 20, 2016, at 1:59 PM, Zachary Turner <zturner at google.com>
> wrote:
> > >
> > >
> > >
> > > On Tue, Sep 20, 2016 at 1:55 PM Greg Clayton <gclayton at apple.com>
> wrote:
> > >
> > > > On Sep 20, 2016, at 1:45 PM, Zachary Turner <zturner at google.com>
> wrote:
> > > >
> > > > I do agree that asserts are sometimes used improperly.  But who's to
> say that the bug was the assert, and not the surrounding code?  For
> example, consider this code:
> > > >
> > > > assert(p);
> > > > int x = *p;
> > >
> > > Should be written as:
> > >
> > > assert(p);
> > > if (!p)
> > >     do_something_correct();
> > > else
> > >     int x = *p;
> > >
> > > >
> > > > Should this assert also not be here in library code?  I mean it's
> obvious that the program is about to crash if p is invalid.  Asserts should
> mean "you're about to invoke undefined behavior", and a crash is *better*
> than undefined behavior.  It surfaces the problem so that you can't let it
> slip under the radar, and it also alerts you to the point that the UB is
> invoked, rather than later.
> > > >
> > > > What about this assert?
> > > >
> > > > assert(ptr);
> > > > int x = strlen(ptr);
> > > >
> > > > Surely that assert is ok right?  Do we need to check whether ptr is
> valid EVERY SINGLE TIME we invoke strlen, or any other function for that
> matter?  The code would be a disastrous mess.
> > >
> > > Again, check before you call if this is in a shared library! What is
> so hard about that? It is called software that doesn't crash.
> > >
> > > assert(ptr)
> > > int x = ptr ? strlen(ptr) : 0;
> > >
> > > I find it hard to believe that you are arguing that you cannot EVER
> know ANYTHING about the state of your program.  :-/
> > >
> > > This is like arguing that you should run a full heap integrity check
> every time you perform a memory write, just to be sure you aren't about to
> crash.
> > >
> > > If you make a std::vector<>, do we need to verify that its internal
> pointer is not null before we write to it?   Probably not, right?  Why
> not?  Because it has a specification of how it works, and it is documented
> that you can construct one, you can use it.
> > >
> > > It's ok to document how functions work, and it is ok to assume that
> functions work the way they claim to work.
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160920/caba101d/attachment-0001.html>