[Lldb-commits] [PATCH] D66174: [Utility] Reimplement RegularExpression on top of llvm::Regex

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

I like the idea of using something from llvm instead of rolling our own. The code changes look relatively simple and straightforward, so that's good.



================
Comment at: lldb/source/Commands/CommandObjectBreakpoint.cpp:685
         RegularExpression regexp(m_options.m_func_regexp);
-        if (!regexp.IsValid()) {
-          char err_str[1024];
-          regexp.GetErrorAsCString(err_str, sizeof(err_str));
+        if (auto error = regexp.GetError()) {
           result.AppendErrorWithFormat(
----------------
labath wrote:
> I wouldn't use `auto` in cases like these because `Optional<string>` is definitely not among the things one would expect a function called `GetError` to return. Though maybe that means the function should really return an llvm::Error?
If Pavel hadn't pointed this out, I would have thought that `error` was an `llvm::Error` here and not a an Optional. I think something should change here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66174/new/

https://reviews.llvm.org/D66174