[lldb-dev] clang::VersionTuple

[Pavel Labath via lldb-dev]

Thanks. I am going to submit the patch then.

On Fri, 15 Jun 2018 at 19:56, Jim Ingham <jingham at apple.com> wrote:
> > On Jun 15, 2018, at 3:44 AM, Pavel Labath via lldb-dev <lldb-dev at 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.