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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
