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

[Slava Gurevich via Phabricator via lldb-commits]

fixathon marked 6 inline comments as done.
fixathon added a comment.

Respond 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;
----------------
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?


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:281
   std::string path_str = path.str();
-  bool success 
-      = CPlusPlusLanguage::ExtractContextAndIdentifier(path_str.c_str(),
-                                                       context,
-                                                       identifier);
+  bool success = CPlusPlusLanguage::ExtractContextAndIdentifier(
+      path_str.c_str(), context, identifier);
----------------
jingham wrote:
> If you use 
> 
> llvm-project/clang/tools/clang-format/git-clang-format
> 
> to clang-format your changes, then it will only apply to your diff, and you won't pick up spurious changes like this that make diffs harder to read.
Thank you for that. Should I attempt to undo the spurious changes for this commit or just keep it in mind for the future?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130098