[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

A few simple changes from inlined comments. Other than that looks good. Should get another OK from others. Also not sure if the JSON stuff in llvm needs to be done separately?



================
Comment at: lldb/include/lldb/Target/Trace.h:115
+  ///   implementation, or an error object in case of failures.
+  virtual llvm::Expected<std::shared_ptr<TraceSettingsParser>>
+  ParsePluginSettings(Debugger &debugger) = 0;
----------------
would std::unique_ptr<> might be better? Are we actually handing out multiple instances of this and sharing it anywhere?


================
Comment at: lldb/include/lldb/lldb-private-interfaces.h:14
 
+#include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-enumerations.h"
----------------
We should avoid this import and use forward declaration here below the includes:

```
namespace llvm {
  namespace json {
    class Object;
  }
}
```


================
Comment at: lldb/source/Commands/Options.td:205-206
   def breakpoint_set_file_colon_line : Option<"joint-specifier", "y">, Group<12>, Arg<"FileLineColumn">,
-    Required, Completion<"SourceFile">, 
-    Desc<"A specifier in the form filename:line[:column] for setting file & line breakpoints.">;  
+    Required, Completion<"SourceFile">,
+    Desc<"A specifier in the form filename:line[:column] for setting file & line breakpoints.">;
   /* Don't add this option till it actually does something useful...
----------------
revert whitespace only changes.


================
Comment at: lldb/source/Commands/Options.td:754
   def source_list_file_colon_line : Option<"joint-specifier", "y">, Group<5>,
-    Arg<"FileLineColumn">, Completion<"SourceFile">, 
+    Arg<"FileLineColumn">, Completion<"SourceFile">,
     Desc<"A specifier in the form filename:line[:column] from which to display"
----------------
revert whitespace only changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705