[Lldb-commits] [PATCH] D33167: Get rid of some uses of StringConvert and reduce some indentation

[Jim Ingham via Phabricator via lldb-commits]

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