> On Sep 20, 2016, at 2:49 PM, Mehdi Amini <mehdi.am...@apple.com> wrote: > > >> 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.
doesn't StringRef store an actual pointer to ""? This would mean StringRef::data() would return non null, but StringRef::size() would return 0. So I believe that isn't a problem with StringRef > > 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> wrote: >> >> > On Sep 20, 2016, at 2:31 PM, Greg Clayton <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> 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> 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> wrote: >> >>> >> >>> >> >>> >> >>> On Tue, Sep 20, 2016 at 1:55 PM Greg Clayton <gclay...@apple.com> wrote: >> >>> >> >>>> On Sep 20, 2016, at 1:45 PM, Zachary Turner <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
