Looks ok, a few things to fix: Change any "assert(false)" code to be "assert(false && "explanation of why we are asserting")". You did this in a few places, but do let people know what the assertion is firing.
You added a few forward class definitions, please move these over to "lldb-forward.h" if needed and include that file if it already isn't. You very soon need to be able to demangle names, and the MSVC version has this commented out so no demangling will occur. A set of open source sources for demangling are at: http://llvm.org/svn/llvm-project/libcxxabi/trunk The file you will want is "src/cxa_demangle.cpp". We used to inline this into our source code. You will need to change the namespace to not use the reserved compiler namespaces for the standard library if you do inline this. Greg On Sep 6, 2013, at 10:15 PM, Virgile Bello <[email protected]> wrote: > Here is an updated MSVC2013 patch with what's not merged yet. Much smaller > than before! > > > On Thu, Sep 5, 2013 at 2:07 AM, Greg Clayton <[email protected]> wrote: > Looks fine to me. > > On Sep 4, 2013, at 9:19 AM, Virgile Bello <[email protected]> wrote: > > > Would such a patch be OK for getopt? (with OptionParser.h) > > Didn't try to hide actual getopt architecture, but at least it prevents > > <getopt.h> from being included directly. > > > > If this gets accepted, the remaining part of MSVC patch should be quite > > small. > > > > > > On Thu, Aug 29, 2013 at 5:02 AM, Greg Clayton <[email protected]> wrote: > > Yes in general to abstract ourselves from the system on which we are > > compiling: > > > > 1 - First try and use LLVM functionality where it makes sense > > 2 - Use LLDB Host layer > > 3 - Use #ifdef in .cpp files only if possible > > 4 - Use $ifdef in .h files as a last resort. > > > > To get back to the "getopt.h" stuff, it would be great to make a Host class > > that abstracts us from the "getopt.h" functionality. > > > > Something like "include/lldb/Host/OptionParser.h" and then a .cpp file in > > common that hides the abstraction for the actual unix version that is used > > for everything but windows, and a windows specific one that could use a > > compatibility layer... > > > > Greg > > > > On Aug 28, 2013, at 9:35 AM, Virgile Bello <[email protected]> wrote: > > > > > Ha yes sure, I thought that was not recommended since LLVM was not used > > > for many other similar situations (i.e. Mutex, Regex, Argument parsing vs > > > getopt, some path functions, etc...). > > > But I suppose it is maybe due more to the fact it wasn't good/stable > > > enough at the time you needed it, and switch would be OK now? > > > > > > > > > On Thu, Aug 29, 2013 at 1:07 AM, João Matos <[email protected]> > > > wrote: > > > +#ifdef _MSC_VER > > > + InterlockedIncrement(&m_last_revision); > > > +#else > > > __sync_add_and_fetch(&m_last_revision, +1); > > > +#endif > > > > > > I see this pattern ifdef'd in a lot of places, I think we should abstract > > > it in an helper "atomics" function, or even better, just re-use LLVM > > > support libraries (llvm::sys::AtomicIncrement). > > > > > > > > > -- > > > João Matos > > > > > > _______________________________________________ > > > lldb-commits mailing list > > > [email protected] > > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > > > > > > <lldb-getopt.diff> > > > <lldb-msvc12-v3.diff> _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
