fixathon marked 3 inline comments as done.
fixathon added a comment.
Replied to comments
================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4685
+ lldb::tid_t m_thread_id = LLDB_INVALID_THREAD_ID;
+ uint32_t m_thread_index = 0;
std::string m_thread_name;
----------------
fixathon wrote:
> jingham wrote:
> > clayborg wrote:
> > > jingham wrote:
> > > > Because of the way CommandObjects work, the constructed values of its
> > > > ivars are never used. You have to call OptionParsingStarting before
> > > > doing any work with the object, so the values set in that function are
> > > > the real ones. There's no actual point in initializing these
> > > > variables, but it mostly doesn't hurt except if you set them to
> > > > different values from the ones in OptionParsingStarting, in which case
> > > > they could cause confusion in people reading the code. Here you've set
> > > > m_thread_index to 0 but the correct starting value is actually
> > > > UINT32_MAX as you can see from the OptionParsingStarting just above.
> > > >
> > > > Can you go through all of the CommandObject subclass ivars that you've
> > > > given values to and make sure they are the ones in the class's
> > > > OptionParsingStarting? Thanks.
> > > yeah, I would like to get everything initialized if we can to limit our
> > > static analysis warnings.
> > If we just called OptionParsingStarting in the constructor would your
> > static analysis tools be smart enough to see that they were initialized?
> > That's the initialization that actually matters so it would be better not
> > to have to write the values in two places (one of which looks like it
> > matters but actually doesn't...)
> I believe so. Would you like to make that change then?
On a second thought, OptionParsingStarting() is a virtual function override.
Calling virtual functions from constructor is bad practice.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130098/new/
https://reviews.llvm.org/D130098
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
