bulbazord accepted this revision. bulbazord added a comment. This revision is now accepted and ready to land.
In D151497#4374495 <https://reviews.llvm.org/D151497#4374495>, @JDevlieghere wrote: > In D151497#4374080 <https://reviews.llvm.org/D151497#4374080>, @bulbazord > wrote: > >> I think it's good to improve the error messaging but I think we can probably >> do better. "Function caller" is specific to the internals of LLDB and isn't >> really meaningful for many users. It's also somewhat confusing from a user's >> perspective when the expression you're running isn't calling a function. >> >> If I were an end user who didn't know much about LLDB, I would think "I'm >> trying to print a variable, what's this about a function?" and "Why is >> memory allocation involved?". I would suggest changing the error message to >> something like: "Unable to evaluate expression while the process is $STATE: >> the process must be running and stopped to evaluate this expression". >> >> What do you think? > > I very much agree that error messages should first and foremost be helpful to > our users. In this particular patch, we have two places where we emit this > error. In `UtilityFunction::MakeFunctionCaller` I believe the current error > is totally appropriate. That doesn't mean that I think it should be shown to > the user as such. I didn't look at how this function is called, but if it > trips, I would like to see an error along the lines of: > > error: Couldn't run utility function. Can't make a function caller while > the process is stopped: the process must be stopped to allocate memory. > > On the other hand, in `UserExpression::Evaluate` I think it's totally > appropriate to rephrase this, but at the same time I don't think we need to > dumb this down. > > error: Unable to evaluate expression while the process is $STATE: the > process must be stopped because the expression might requires allocating > memory. > > I'll update the error messages accordingly. This makes sense to me. I don't want to dumb things down per-se, but now that I look back at my suggestion it may have been too simplistic. I think the message you've written is an excellent. Thank you for working on this. ================ Comment at: lldb/source/Expression/UserExpression.cpp:207-208 + + // Since we might need to call allocate memory, we need to be stopped to run + // an expression. if (process != nullptr && process->GetState() != lldb::eStateStopped) { ---------------- ================ Comment at: lldb/source/Expression/UtilityFunction.cpp:68-69 } // Since we might need to call allocate memory and maybe call code to make // the caller, we need to be stopped. if (process_sp->GetState() != lldb::eStateStopped) { ---------------- CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151497/new/ https://reviews.llvm.org/D151497 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
