[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

Thanks for the feedback (addressed below).

btw, where should this module go?

Since it's intended for clang, and clang based tool, developers, I put it in `clang/utils`, but Zackery suggested `lldb/examples/python` might be a better place.  Please let me know if anyone has a preference.



================
Comment at: utils/clangdiag.py:66
+def setDiagBreakpoint(frame, bp_loc, dict):
+    id = frame.FindVariable("DiagID").GetValue()
+    if id is None:
----------------
clayborg wrote:
> Is there only ever one of these? If so this is good. 
Currently, DiagnosticsEngine::Report only has two signatures, and both contain an `unsigned DiagID` parameter, so this is just a sanity check to make sure we are in the right place.



================
Comment at: utils/clangdiag.py:117
+        disable(exe_ctx)
+    else:
+        getDiagtool(exe_ctx.GetTarget(), args.path[0])
----------------
clayborg wrote:
> should probably verify that this 'diagtool' with:
> 
> ```
> elif args.subcommands == 'diagtool':
> ```
> and add an else that creates an error:
> ```
> else:
>     result.SetError("invalid subcommand '%s'" % (args.subcommands))
> ```
This is already handled by `argparser`, which will raise an exception if the first parameter isn't one of (enable, disable, daigtool).  That's the benefit of using `subparsers`.  So, by the time you get to this point, it must be "diagtool".

However, I think I should probably verify the path past actually exists.  Plus, I think I could add a better exception to alert the user how to fix the problem if diagtool couldn't be found in the callback.  Suggestions welcome.


https://reviews.llvm.org/D36347