How about making it a std::unique_ptr<std::atomic<lldb::addr_t>>? This way there's no risk of re-introducing a potential race, and copying still works.
On Mon, Aug 29, 2016 at 8:49 AM Zachary Turner <ztur...@google.com> wrote: > My concern is that by not taking out the copy constructor, someone passes > another Address by value later. If passing by value is unsafe (ignoring > whether it generates a compiler error) it should be prevented by the > compiler. That is doing due diligence. > > I will track down the commit that introduced this, but again, all you have > to do is inspect the class to see that it is not thread aware at all. > Anyone attempting to share an Address on multiple threads had better be > using a mutex or they're already gonna have a bad time, in which case the > atomic seems unnecessary > > > > On Mon, Aug 29, 2016 at 8:38 AM Greg Clayton <gclay...@apple.com> wrote: > >> >> > On Aug 26, 2016, at 5:37 PM, Zachary Turner <ztur...@google.com> wrote: >> > >> > How would you enforce that, other than to ask people to try to remember >> not to do it? It seems to me like std::atomic not being copy-constructible >> is telling you that, well, you shouldn't be copying it. >> >> It just won't compile on platforms that have this issue. We are not >> taking the Address copy constructor out just so we can enforce not passing >> this object by value, that is not a viable solution. The copy constructor >> of address currently works around this issue, but the solution is not to >> take out the copy constructor of Address. >> >> > BTW, nobody has commented on my original concern that the atomic may >> not even be necessary in the first place. >> >> Track down the original author and see what the commit message said. I >> can see how this could cause problems, but maybe if we don't allow Address >> to be passed by value the problem goes away. I am not sure. Just taking it >> out because it doesn't compile in your platform is not the way forward >> without doing due diligence. >> >> So just change the places where this gets passed by value and be done >> with it, or take the time to make sure you don't introduce a threading >> issue and decide to take out the std::atomic. >> >> Greg Clayton >> >> >> >> >>
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
