> On Sep 20, 2016, at 2:46 PM, Zachary Turner <ztur...@google.com> wrote: > > Occasionally (and in my experience *very* occasionally), you need to treat "" > as different from null.
return an Optional<StringRef>? — Mehdi > But the frequency with which that is really necessary is much lower than > people realize. It just seems high because of the prevalence of raw pointers. > > For every other case, you can use withNullAsEmpty() (mostly used as a helper > for porting and when dealing with native APIs, which is to say quite a bit > right now, but in the long run, almost never (evidenced by the fact that it > only got added to LLVM a few days ago and nobody has ever needed it). > > On Tue, Sep 20, 2016 at 2:41 PM Mehdi Amini <mehdi.am...@apple.com > <mailto:mehdi.am...@apple.com>> wrote: > > > On Sep 20, 2016, at 2:31 PM, Greg Clayton <gclay...@apple.com > > <mailto:gclay...@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? > > Well no, it is indeed terrible: changing die.GetName() to return StringRef > make the code a lot safer. > > When StringRef is used everywhere, this problem disappear by itself. > StringRef(const char *) is a very unsafe API, that is not intended to be used > *inside* the library (or very sporadically). So if you build a StringRef out > of a const char *, you’re supposed to be in a place that deals with > “uncontrolled” input and you need to sanitize it. > > (Also StringRef(const char *) is “costly": it calls strlen) > > — > Mehdi > > > > > 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 <ztur...@google.com > >> <mailto:ztur...@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 <gclay...@apple.com > >> <mailto:gclay...@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 <ztur...@google.com > >>> <mailto:ztur...@google.com>> wrote: > >>> > >>> > >>> > >>> On Tue, Sep 20, 2016 at 1:55 PM Greg Clayton <gclay...@apple.com > >>> <mailto:gclay...@apple.com>> wrote: > >>> > >>>> On Sep 20, 2016, at 1:45 PM, Zachary Turner <ztur...@google.com > >>>> <mailto:ztur...@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. > >> > > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
