<div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Nov 2, 2018 at 4:08 PM Tim Northover <<a href="mailto:tnorthover@apple.com" target="_blank">tnorthover@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">After that it was discovered OSLog depends on the other format helpers so there was still a circular dependency; I decided adding the others was a minor enough change to just go ahead.<br></blockquote><div>Thanks - I missed the review. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> On 2 Nov 2018, at 15:03, Tim Northover <<a href="mailto:tnorthover@apple.com" target="_blank">tnorthover@apple.com</a>> wrote:<br>
>> (If the do belong here: the namespaces and comments don't seem to have been updated)<br>
> <br>
> Good point, I’ll get onto fixing that.<br>
<br>
Actually, do you have any particular ones in mind? After going in to make changes, they still look pretty good to me; I don’t think libclangAnalysis should have the monopoly on the term.<br></blockquote><div>Thanks for looking at this!</div><div><br></div><div>From my (very limited) understanding of os_log and AST, it's going to rely on __builtin_os_log_format_buffer_size being computed at compile-time, so AST now needs to understand the semantics of format strings.</div><div>In that case, I don't think it makes sense to think of the format string parser as part of the analyzer - as the build deps suggest, it's now part of AST and gets reused by analyzer. (Similar to how the analyzer uses other bits of AST/clang). If there are parts only relevant to analyzer, it'd be nice to move them out of the AST library, but I don't know to what extent that's feasible.</div><div><br></div><div>So it does seem to me like all the uses of analyzer namespaces are suspect - moving code from Analyzer to AST is a semantic difference (the layers aren't *just* about making the linker happy, after all!)</div><div><br></div><div>Cheers, Sam</div></div></div></div>