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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to