[Lldb-commits] [PATCH] D59015: [lldb-mi] Include full path in the -data-disassemble response

Mon Apr 22 07:05:34 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:
> > 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. 

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59015