Fair enough. I figured it was worth asking about, but the rationale makes sense.
On Wed, Nov 18, 2015 at 5:18 PM Jim Ingham <jing...@apple.com> wrote: > > > On Nov 18, 2015, at 4:31 PM, Zachary Turner via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > > > zturner added a reviewer: clayborg. > > zturner added a comment. > > > > +greg since this touches some SB stuff and the ObjectFile plugins. > > > > > > ================ > > Comment at: include/lldb/API/SBProcess.h:345-346 > > @@ +344,4 @@ > > + // Save the state of the process in a core file (or mini dump on > Windows). > > + lldb::SBError > > + SaveCore(const char *file_name); > > + > > ---------------- > > Greg, is there any reason why this couldn't be an `llvm::StringRef`? I > know historically we have used this char* mechanism, but it seems like > `StringRef` could be a better, safer alternative than passing raw strings > through the SB API. As long as you provide an appropriate typemap, > everything should work out. > > Once we pass an llvm::StringRef to an SB API it becomes part of the API, > and changes to llvm::StringRef make us binary incompatible. Maybe > llvm::StringRef won't ever change, but there's no guarantee about that, so > unless we get some very persuasive benefit from this, I'd strongly prefer > to use more vanilla data types. > > Also, I would like to keep the process of compiling against the SB API's > as simple as possible. Right now you don't need the llvm headers to build > an app that just uses the SB API's. Again, the benefit would have to be > fairly great before I'd want to add this requirement. > > If you want to be more formal about this, then have the API take an > SBFileSpec. > > Jim > > > > > > > > > ================ > > Comment at: > packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:20 > > @@ +19,3 @@ > > + @not_remote_testsuite_ready > > + @skipUnlessWindows > > + def test_windows_mini_dump (self): > > ---------------- > > How do you feel about `@skipIf(oslist=not_in(['windows']))`? > > > > The generic skipIf and expectedFailure decorators were extended recently > and are now flexible enough to handle unless cases, so I'm partial to the > idea of eventually killing off the rest of the decorators, and just having > a single one that handles everything. But I want to dogfood the idea first > and see what people think about it :) > > > > ================ > > Comment at: > packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:32 > > @@ +31,3 @@ > > + self.assertTrue(process.Kill().Success()) > > + # Clean up the mini dump file. > > + os.unlink(core) > > ---------------- > > Can you assert true that the file exists? Is there some thing > additional that we can add where you then load the core and try to read > something out (say the executable name), and assert that it's equal to > `a.out`? > > > > Right now all we know is that there's a file. > > > > ================ > > Comment at: > packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:33 > > @@ +32,3 @@ > > + # Clean up the mini dump file. > > + os.unlink(core) > > + > > ---------------- > > This isn't exception safe right now. For example, it won't unlink the > file if one the asserts fails. > > > > ================ > > Comment at: > source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:17 > > @@ +16,3 @@ > > + > > +#ifdef _WIN32 > > +#include "lldb/Host/windows/windows.h" > > ---------------- > > I think it would be better to just not even compile this file on > non-Windows. Exclude it at the CMake level, and have `SaveCore()` return > false directly. In the future we may add a way to deal with mini dumps > outside of the Windows API, and then we can add that as a separate file > like `ObjectFilePECOFFMiniDumpRaw.cpp` which can be used always, and then > delete this one. > > > > > > http://reviews.llvm.org/D14793 > > > > > > > > _______________________________________________ > > lldb-commits mailing list > > lldb-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
