<div dir="ltr">(sorry for the delay). As per my earlier comment now that it seems like the issue Kim was seeing is resolved, feel free to check this in.<div><br></div><div>More comments inline:<br><div class="gmail_extra">
<br><br><div class="gmail_quote">On Mon, Oct 28, 2013 at 8:52 PM, John Thompson <span dir="ltr"><<a href="mailto:john.thompson.jtsoftware@gmail.com" target="_blank">john.thompson.jtsoftware@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">  Sean,<br>
<br>
  Thanks for the excellent review, and the reference to yaml-bench.  It caught a few things so far, and I'll have to use it when I write the other tests too.  It didn't like the backslashes in Windows file paths, or the colons in SourceLocations, so I put in some character replacing and quoting.<br>

<br>
  I revised the for loops as pointed out.<br>
<br>
  Also, I wanted to ask about the special cases, such as where I use values like (null) to represent when a pointer is null, or (invalid) when a SourceLocation has the invalid flag set.  In the former case, passing a null pointer is expected in some situations.  The YAML parser doesn't seem to mind (null), but is this reasonable semantically?<br>
</blockquote><div><br></div><div>I think that YAML's "designated null" is literally `null`, which gets converted into the appropriate language-specific nullish thing <<a href="http://yaml.org/type/null.html">http://yaml.org/type/null.html</a>> (and surprisingly not a string, which is what you would expect... YAML's "you can usually omit the quotes" deal is convenient, but things like this make it a big pain for writing robust software). E.g. in Python:</div>
<div><br></div><div>>>> print(yaml.load('a: null'))</div><div>{'a': None}</div><div><div>>>> print(yaml.load('a: null_'))</div><div>{'a': 'null_'}</div></div><div>
<br></div><div>I'm not sure how/whether LLVM's YAML parser handles this. In the worst case it would get turned into a string which is "no harm done".</div><div><br></div><div>That link though seems to suggest that the canonical YAML form for this is just `~` (canonical YAML roughly means "fully marked up"/"desugared") and also just empty is fine.</div>
<div><br></div><div><div>>>> print(yaml.load('a: ~'))</div><div>{'a': None}</div><div>>>> print(yaml.load('a: '))</div><div>{'a': None}</div></div><div><br></div><div>If our parser accepts either of these, then they might be simpler (I especially like the idea of just leaving it empty). I'm not sure what impact these choices would have on the testing though (e.g. expressing "the field is empty" in FileCheck (maybe a regex like "foo:$" would work)).</div>
<div><br></div><div>Any of these 3 choices would be fine; I think you're in a better position to decide as to which is preferable based on your experience working with/testing the output.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<br>
  Also, again semantically, you probably already noticed I'm not trying to flatten the full data structures for possible later resurrection, just providing what I think is sufficient high-level information to know what the preprocessor is up to.<br>
</blockquote><div><br></div><div>Yeah, I wouldn't worry about going too "deep", as long as the tool serves its purpose. What data structures in particular were you thinking could be flattened? Being able to reconstruct full macro expansion trees from the trace would be nifty, but definitely a project for another day :)</div>
<div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
  Kim,<br>
<br>
  Thanks also for your help. I'll install ninja cmake and give it a shot.<br>
<div class="im"><br>
  -John<br>
<br>
Hi silvas, kimgr,<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2020" target="_blank">http://llvm-reviews.chandlerc.com/D2020</a><br>
<br>
CHANGE SINCE LAST DIFF<br>
</div>  <a href="http://llvm-reviews.chandlerc.com/D2020?vs=5196&id=5210#toc" target="_blank">http://llvm-reviews.chandlerc.com/D2020?vs=5196&id=5210#toc</a><br>
<div class=""><div class="h5"><br>
Files:<br>
  test/pp-trace/pp-trace-macro.cpp<br>
  pp-trace/PPTrace.cpp<br>
  pp-trace/PPCallbacksTracker.h<br>
  pp-trace/PPCallbacksTracker.cpp<br>
  pp-trace/CMakeLists.txt<br>
  pp-trace/Makefile<br>
  CMakeLists.txt<br>
  Makefile<br>
</div></div></blockquote></div><br></div></div></div>