<div dir="ltr">Fair enough.  I figured it was worth asking about, but the rationale makes sense.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Nov 18, 2015 at 5:18 PM Jim Ingham <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On Nov 18, 2015, at 4:31 PM, Zachary Turner via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
><br>
> zturner added a reviewer: clayborg.<br>
> zturner added a comment.<br>
><br>
> +greg since this touches some SB stuff and the ObjectFile plugins.<br>
><br>
><br>
> ================<br>
> Comment at: include/lldb/API/SBProcess.h:345-346<br>
> @@ +344,4 @@<br>
> +    // Save the state of the process in a core file (or mini dump on Windows).<br>
> +    lldb::SBError<br>
> +    SaveCore(const char *file_name);<br>
> +<br>
> ----------------<br>
> 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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
If you want to be more formal about this, then have the API take an SBFileSpec.<br>
<br>
Jim<br>
<br>
><br>
><br>
><br>
> ================<br>
> Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:20<br>
> @@ +19,3 @@<br>
> +    @not_remote_testsuite_ready<br>
> +    @skipUnlessWindows<br>
> +    def test_windows_mini_dump (self):<br>
> ----------------<br>
> How do you feel about `@skipIf(oslist=not_in(['windows']))`?<br>
><br>
> 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 :)<br>
><br>
> ================<br>
> Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:32<br>
> @@ +31,3 @@<br>
> +        self.assertTrue(process.Kill().Success())<br>
> +        # Clean up the mini dump file.<br>
> +        os.unlink(core)<br>
> ----------------<br>
> 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`?<br>
><br>
> Right now all we know is that there's a file.<br>
><br>
> ================<br>
> Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:33<br>
> @@ +32,3 @@<br>
> +        # Clean up the mini dump file.<br>
> +        os.unlink(core)<br>
> +<br>
> ----------------<br>
> This isn't exception safe right now.  For example, it won't unlink the file if one the asserts fails.<br>
><br>
> ================<br>
> Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:17<br>
> @@ +16,3 @@<br>
> +<br>
> +#ifdef _WIN32<br>
> +#include "lldb/Host/windows/windows.h"<br>
> ----------------<br>
> 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.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D14793" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14793</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
<br>
</blockquote></div>