jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
This looks fine. Like Kamil, I think it would help to document your new
interfaces.
Going away from StringConvert, you are switching from an API that gives a value
on fail to one that doesn't change the input value on error. You mostly handle
the failures right away, so in that case it's fine to just declare them. But
anywhere it's not immediately clear you are directly returning, you should
probably initialize the variables to the fail value.
================
Comment at: lldb/include/lldb/Interpreter/CommandReturnObject.h:132-154
+ template <typename... Args>
+ bool AppendError(lldb::ReturnStatus Status, const char *Format,
+ Args &&... args) {
+ SetStatus(Status);
+ AppendErrorWithFormatv(Format, std::forward<Args>(args)...);
+ return Succeeded();
+ }
----------------
Can you add a comment saying what these functions return? It's clear now that
the code is in the header file, but that doesn't describe a contract just a
current state, and anyway, will keep it clear if the code gets moved to the
implementation file.
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1178-1181
+ if (auto ST = process->Signal(signo))
+ return result.AppendError(eReturnStatusFailed,
+ "Failed to send signal {0}: {1}\n", signo,
+ ST.AsCString());
----------------
"ST" is not how we name variables in lldb. Besides the capitalization, which
is wrong, we used to call all Error variables "error" which gave its use a bit
of consistency. You didn't change the variable names when you renamed them to
"error" which is probably why calling this one "error" gave you pause. But we
should maintain consistency for now, and that will make it easier to go rename
them to whatever is appropriate when we get around to that.
================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2687-2688
+ bool success = false;
+ addr_t load_addr;
+ if (!llvm::strton(load_addr_cstr, load_addr)) {
+ result.AppendError(eReturnStatusFailed,
----------------
I don't think it does any harm here. But your switching from an API that sets
a fail value to one that doesn't, so you're adding the possibility of
uninitialized variable access. Probably good as you are making this transition
to initialize to the fail value.
https://reviews.llvm.org/D33167
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
