Thanks for the suggestions! Not sure if I'll be able to make those changes before tomorrow EOD, and after that I will be out for ~2 weeks, so if time doesn't permit, I may not follow up with another patch until after I get back.
On Tue, Mar 21, 2017 at 11:19 AM Jim Ingham via Phabricator < revi...@reviews.llvm.org> wrote: > jingham added a comment. > > None of this isn't modeling the situation particularly clearly, since we > have "architecture specific modifications to general behaviors" that aren't > coming in through plugins so that it would be easy to modify the behavior > for new architectures. Instead, we're requiring new architectures to go in > and edit supposedly generic code. We put off that abstraction because it > was unclear what we would need it to do, and because of that for the most > part we just put these architecture specific behaviors inline in various > places. Then this one callback had nowhere to go but ArchSpec which is the > only logical place for architecture specific code to accumulate. It > doesn't seem like we should gate cleaning up ArchSpec on that larger issue, > however. > > Rather than introducing another file, it seems simpler to make this part > of StopInfo, since it is logically modifying the StopInfo for a thread. It > isn't right there, but it's any worse, and StopInfo is closer to the > machine (though so far mostly closer to the Platform) than Process should > be. And StopInfo's already have to know about pretty much all the details > of Process/Thread/etc... so you wouldn't be adding knowledge to them that > isn't appropriate. > > I'd also suggest removing the "callback" name from it since it really > isn't a general purpose callback, it is absolutely determined by the > ArchSpec. For instance if you were to use anything but the ARM one for ARM > debugging you would break debugging. It's not optional... Instead I'd > call it something like InvokeStopInfoArchOverride. That will also make it > clear that this is a target for gathering up into some "ArchSpec specific > behaviors" if/when we get around to that. > > > https://reviews.llvm.org/D31172 > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
