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

[Greg Clayton via Phabricator via cfe-commits]

clayborg accepted this revision.
clayborg added inline comments.


================
Comment at: utils/clangdiag.py:117
+        disable(exe_ctx)
+    else:
+        getDiagtool(exe_ctx.GetTarget(), args.path[0])
----------------
hintonda wrote:
> 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.
nice, I didn't realized that argparser would protect against that. Checking the file exists would be a good idea.


https://reviews.llvm.org/D36347