[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response
Greg Clayton via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 26 14:03:25 PDT 2019
clayborg added inline comments.
================
Comment at: lldb/tools/lldb-mi/MICmdCmdData.cpp:419
+ // Get a full path to the file.
+ std::unique_ptr<char[]> pPathBuffer(new char[PATH_MAX]);
+ lineEntry.GetFileSpec().GetPath(pPathBuffer.get(), PATH_MAX);
----------------
anton.kolesov wrote:
> clayborg wrote:
> > anton.kolesov wrote:
> > > clayborg wrote:
> > > > Confused as to why we are calling malloc and free here for pPathBuffer? Why not just:
> > > > ```
> > > > char pPathBuffer[PATH_MAX];
> > > > ```
> > > I don't have a strong opinion on this, so to maintain consistency in the code I'm trying to use what other code in lldb-mi uses in similar situations - which is either unique_ptr or static local variable, but I presume it was decided that second approach is not good. FWIW, If I were to write code without regard of what is being done in the same project, then I would never ever had a variable named "miValueConst5".
> > I would just use a local variable on the stack as I suggested. static variables are not correct and should never be used in cases like this, so if you see other errors, might be a good idea to submit a patch and improve lldb-mi. I am all for consistency where it makes sense, but I am also for fixing issues when we see them.
> But what is wrong with the approach of using std::unique_ptr<char[]> local variable?
That forces a call to malloc(...) and free(...). No need to involve allocations on the heap for such simple things
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59015/new/
https://reviews.llvm.org/D59015
More information about the lldb-commits
mailing list