Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Saleem Abdulrasool via lldb-commits]

compnerd added a comment.

Ah, I just ran git-clang-format, and that found a few additional things.  
Incorporated that; Ill commit this tonight and hopefully have the second pass 
done soon.


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Zachary Turner via lldb-commits]

zturner added a comment.

This looks good, no probelms on Windows that I can see.  I ran clang-format on 
your patch just to confirm, and it fixed up some trailing whitespace at the end 
of lines.  I'm not sure if the problem was in the generation of the patch on 
your end, the application of the patch on my end, or if the whitespace is 
really there on your end too, but just make sure it's all clean.  Quick example 
just so you can look at this specific line and see if it's on your end:

   // Other libraries and framework includes
   // Project includes
  diff --git a/include/lldb/Core/History.h b/include/lldb/Core/History.h
  index 001ad3e..164d1bf 100644
  --- a/include/lldb/Core/History.h
  +++ b/include/lldb/Core/History.h
  @@ -46,7 +46,7 @@ public:
   // onto the end of the history stack.
  
   virtual HistoryEvent
  -CreateHistoryEvent () = 0;
  +CreateHistoryEvent () = 0;

This line had whitespace at the end when I applied the patch.


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Zachary Turner via lldb-commits]

If you haven't or can't run it on Windows, then give me a chance to test it
on Windows before you commit.  I can do it tomorrow unless you are set up
to do it instead.

On Mon, May 16, 2016 at 6:13 PM Saleem Abdulrasool 
wrote:

> compnerd added a comment.
>
> I ran it on Darwin while I was working on this.  Ill run it on Linux once
> before I commit the first pass.
>
>
> http://reviews.llvm.org/D20278
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Saleem Abdulrasool via lldb-commits]

compnerd added a comment.

I ran it on Darwin while I was working on this.  Ill run it on Linux once 
before I commit the first pass.


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Zachary Turner via lldb-commits]

zturner added a comment.

Which platforms did you run the test suite on?


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Saleem Abdulrasool via lldb-commits]

compnerd added a comment.

Zach is correct, this doesn't *remove* the Mutex and Condition types, merely 
starts reducing the easier uses of it.

My thinking is to do this piecemeal, slowly reducing the local usage until the 
real uses remain in the cases where we need to actually change the structure of 
the code.  In particular, it seems that there are places where we have 
`Mutex::Locker` passed in by reference to get a lock.

I agree that we want to start moving towards using std::condition_variable, 
however, that was a bit more involved as that will require adding a conversion 
operator for TimeValue.  Switching to `std::condition_variable` requires the 
switch to `std::mutex` as `std::conditional_variable::wait` requires a 
`std::unique_lock`.

I wanted to get as much of the mechanical change out of the way as possible so 
that when the rest of the uses are removed, it is easier to actually review the 
change to ensure that no subtle bugs are being introduced since it won't be as 
mechanical.


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Kamil Rytarowski via lldb-commits]

krytarowski added a comment.

There is `Mutex::` used in my code. And it's going to be removed soon.


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Zachary Turner via lldb-commits]

zturner added a comment.

Looking at the 2 patches, they seem to be completely independent of each
other. Would this patch going in break you somehow?


http://reviews.llvm.org/D20278



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D20278: first pass for removing Mutex for std::{, recursive_}mutex

[Zachary Turner via lldb-commits]

Looking at the 2 patches, they seem to be completely independent of each
other. Would this patch going in break you somehow?
On Mon, May 16, 2016 at 12:02 AM Kamil Rytarowski  wrote:

> krytarowski added a subscriber: krytarowski.
> krytarowski added a comment.
>
> Looks good, however could we merge firstly Plugins/Process/NetBSD
> http://reviews.llvm.org/D20274 ? Otherwise I will need to keep syncing my
> patch forever for generic changes.
>
> Thanks!
>
>
> http://reviews.llvm.org/D20278
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits