[Lldb-commits] [PATCH] D130098: [LLDB][NFC][Reliability] Fix uninitialized variables from Coverity scan

[Slava Gurevich via Phabricator via lldb-commits]

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