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