+1 for limiting the scope of a variable as much as possible <br><div class="gmail_quote"><div dir="ltr">On Mon, Jun 18, 2018 at 7:57 AM Pavel Labath via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks. I am going to submit the patch then.<br>
<br>
On Fri, 15 Jun 2018 at 19:56, Jim Ingham <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br>
> > On Jun 15, 2018, at 3:44 AM, Pavel Labath via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br>
> ><br>
> > Hello again,<br>
> ><br>
> > Just a quick update on the state of this.<br>
> ><br>
> > I've managed to move VersionTuple from clang to llvm. I've also<br>
> > created <<a href="https://reviews.llvm.org/D47889" rel="noreferrer" target="_blank">https://reviews.llvm.org/D47889</a>> to switch over our version<br>
> > handling to that class.<br>
> ><br>
> > Could I interest anyone in taking a quick look at the patch?<br>
><br>
><br>
> Somehow I can’t log into Phabricator from home so I can’t comment right now, but I took a look.<br>
><br>
> In some of your changes in the SB files you do:<br>
><br>
>   if (PlatformSP platform_sp = GetSP())<br>
>     version = platform_sp->GetOSVersion();<br>
><br>
> 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:<br>
><br>
>   PlatformSP platform_sp = GetSP()<br>
>   if (platform_sp)<br>
>     version = platform_sp->GetOSVersion();<br>
><br>
> so switching to the other form in a couple of places only kinda forces the double-take.  But that’s a little nit.<br>
<br>
I've rechecked the llvm style guide. It doesn't say anything about<br>
this particular issue, but this syntax is used throughout the examples<br>
demonstrating other things.<br>
<br>
What I like about this syntax is that it makes it clear that the<br>
variable has no meaning outside of the if block, which is the same<br>
reason we declare variables inside the for() statement. But those are<br>
microscopic details I'd leave to the discretion of whoever is writing<br>
the code.<br>
_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
</blockquote></div>