[Lldb-commits] [PATCH] D68434: SBFile support in SBCommandReturnObject

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some nits inline, otherwise it looks fine to me.



================
Comment at: lldb/include/lldb/Utility/ReproducerInstrumentation.h:243
+template <class T> struct serializer_tag {
+  typedef typename std::conditional<std::is_trivially_copyable<T>::value, ValueTag, NotImplementedTag>::type type;
+};
----------------
clang-format this


================
Comment at: lldb/source/API/SBCommandReturnObject.cpp:118-142
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
   LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
-
   if (fh) {
     size_t num_bytes = GetOutputSize();
     if (num_bytes)
       return ::fprintf(fh, "%s", GetOutput());
   }
----------------
Have these methods delegate to one another (I'm not sure which should be the "base" one TBH).


================
Comment at: lldb/source/API/SBCommandReturnObject.cpp:133
+    return 0;
+  return file_sp->Printf("%s", GetOutput());
+}
----------------
Why not just `file_sp->Write(GetOutput(), GetOutputSize())` ? It invoking formatting function for this seems overkill (and ideally I would like to remove formatted output capabilities from the file class completely).


================
Comment at: lldb/source/API/SBCommandReturnObject.cpp:275-284
+  FileSP file = std::make_shared<NativeFile>(fh, transfer_ownership);
+  ref().SetImmediateOutputFile(file);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh,
                                                   bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
----------------
Here the need isn't as pressing, but I'd probably have these delegate too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68434