[Lldb-commits] [PATCH] D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 2 inline comments as done.
mib 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76188/new/

https://reviews.llvm.org/D76188