Thanks. I am going to submit the patch then. On Fri, 15 Jun 2018 at 19:56, Jim Ingham <jing...@apple.com> wrote: > > On Jun 15, 2018, at 3:44 AM, Pavel Labath via lldb-dev > > <lldb-dev@lists.llvm.org> wrote: > > > > Hello again, > > > > Just a quick update on the state of this. > > > > I've managed to move VersionTuple from clang to llvm. I've also > > created <https://reviews.llvm.org/D47889> to switch over our version > > handling to that class. > > > > Could I interest anyone in taking a quick look at the patch? > > > Somehow I can’t log into Phabricator from home so I can’t comment right now, > but I took a look. > > In some of your changes in the SB files you do: > > if (PlatformSP platform_sp = GetSP()) > version = platform_sp->GetOSVersion(); > > I don’t like putting initializers in if statements like this because I always > have to think twice about whether you meant “==“. Moreover, all of the > surrounding code does it differently: > > PlatformSP platform_sp = GetSP() > if (platform_sp) > version = platform_sp->GetOSVersion(); > > so switching to the other form in a couple of places only kinda forces the > double-take. But that’s a little nit.
I've rechecked the llvm style guide. It doesn't say anything about this particular issue, but this syntax is used throughout the examples demonstrating other things. What I like about this syntax is that it makes it clear that the variable has no meaning outside of the if block, which is the same reason we declare variables inside the for() statement. But those are microscopic details I'd leave to the discretion of whoever is writing the code. _______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
