jingham added inline comments.
================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:880
auto func =
- RegularExpressionSP(new RegularExpression(m_options.m_function));
+ RegularExpressionSP(new
RegularExpression(m_options.m_symbols.front()));
StackFrameRecognizerManager::AddRecognizer(recognizer_sp, module, func);
----------------
mib wrote:
> jingham wrote:
> > mib wrote:
> > > jingham wrote:
> > > > labath wrote:
> > > > > mib wrote:
> > > > > > labath wrote:
> > > > > > > mib wrote:
> > > > > > > > labath wrote:
> > > > > > > > > Is there something which ensure that m_symbols contains at
> > > > > > > > > least one element here? (i.e., that we do not silently drop
> > > > > > > > > the extra symbols arguments)
> > > > > > > > Few lines above, we make sure the symbols list is not empty.
> > > > > > > Not empty is one thing, but what about the case when it contains
> > > > > > > 2 or more elements (`frame recognizer add --name foo --name bar
> > > > > > > --regex`)? Will that produce some kind of an error, or will it
> > > > > > > just ignore the second regex ?
> > > > > > It takes only the first symbol and ignores the other ones.
> > > > > Hmm.. that doesn't sound very useful. I think that should be an error.
> > > > Just put --name and --regex in different option groups when you define
> > > > the command. Then the command machinery won't allow you to specify
> > > > both at the same time (and the help will show they are mutually
> > > > incompatible).
> > > >
> > > > I agree with Pavel, an error is what you want in that case.
> > > @jingham I think this is wrong because `--regex` is just a boolean flag,
> > > not the actual regular expression.
> > >
> > > When this flag is enabled, the overloading for `AddRecognizer`that is
> > > called is the one that holds `RegularExpressionSP`s instead of the
> > > overloading with a `ConstString` and `llvm::ArrayRef<ConstString>` ...
> > >
> > > I'll add the check above to make sure `m_symbols` has only 1 entry when
> > > the `--regex` flag is enable and return an error otherwise.
> > Oh, that's unfortunate. It's weird that you can pass --name twice w/o the
> > --regex but only once with. But it's probably too late to change that now.
> >
> > You solution sounds fine, though you should also amend the help string for
> > --name to say "can be specified more than once except if --regex is
> > provided" or this will be really confusing. BTW, if there any reason why
> > this can't take more than one regex? I allowed only one for breakpoints
> > and things like the "no step into source regex" on the theory that you can
> > always just use "|" to concatenate two regex's. That is in the end
> > probably more efficient but for maintenance it's easier if your regex's
> > don't get too long, so I have wished on occasion since then that I had
> > allowed more than one regex for these options.
> >
> > Note, the reason I don't like "--regex" as a flag is it makes the job of
> > providing completions for --name harder. Note, it's still possible if the
> > user typed:
> >
> > (lldb) whatever --regex --name A<TAB>
> >
> > because the option completion for --name can actually look at the whole
> > command line. For instance,
> >
> > --shlib libBar.dylib --name Foo<TAB>
> >
> > will limit the completions to names in libBar. But that makes it trickier
> > to do.
> >
> > For folks designing commands, IMO would be better to have --name <NAME> and
> > --regex <REGEX> and not have the --regex be just a flag.
> I didn't know that completion could be restricted by other flags, this sounds
> like a neat "optimization".
>
> The issue with making the `--regex` as the actual regular expression is that
> it will prevent us to distinguish the shared library regex from the symbol
> regex.
>
> I'm not against changing that behavior to have better completion and more
> homogeneous commands but this would need to be done on a separate patch.
It's fine to do that as a separate patch, or leave it for later as your time
dictates. This feature doesn't demand it.
But you should document the current behavior in the option string for --name.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76188/new/
https://reviews.llvm.org/D76188
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
