<div dir="ltr">On Thu, Jun 20, 2013 at 6:41 PM, Thompson, John <span dir="ltr"><<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote"><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">





<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Sean,</span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Thanks for your efforts in this.  As you can see, I’m struggling to become comfortable with this style of coding and using STL, but with your help I’m starting
 to learn.</span></p></div></div></blockquote><div><br></div><div style>Cool. I'm glad I'm able to help.</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">
<div lang="EN-US" link="blue" vlink="purple"><div class="im">
<p class="">>Again, this function is not appropriate. Please investigate the implementation of clang's macro-related diagnostic messages for inspiration, or consult with cfe-dev with a more refined question than your last question (see below; I think
 I may have identified what the key question is); the appropriate solution will almost surely require significant changes to your patch. Clang gives detailed notes in diagnostics if the offending code was expanded from a macro, and traces the expansion many
 levels deep in some cases.</p>
</div><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I will continue to look into this tomorrow.  Today I just pretty much rewrote a lot of the code per your comments about style and conventions.</span></p>

<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Previously you mentioned that I should make smaller patches, which implies an iterative approach.  Please consider this a step in the process.  The bottom line
 is that even though this approach is flawed, it nonetheless mostly works.</span></p></div></blockquote><div><br></div><div style>Iterative doesn't mean committing code that is known to be using a flawed approach. Iterative means committing the smallest possible piece of code along the *right* approach. A *huge* part of it is interacting with the community to gain consensus about the right approach and cleaning up existing code to make the right approach possible.</div>
<div style><br></div><div style>Now that I look at Modularize.cpp a bit more closely, one of the first obvious incremental steps to take is to factor the existing code so that the checks are cleanly separated. I expect that there are *at least* 5 patches' worth of incremental cleanups and refactoring to be made there.</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"><div lang="EN-US" link="blue" vlink="purple"><div>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">  Note that having the preprocessed conditional is primarily for the benefit of the user in displaying the message to help him understand which macro is different.  An alternative would
 have been to modify the PPCallbacks mechanism to pass along a flag indicating the resulting condition Boolean value, rather than my using a string compare. </span></p></div></div></blockquote><div><br></div><div style>That sounds like a better approach (it sounds like Argyrios had some good feedback; you probably should continue that discussion instead of leaving him hanging!). AFAIK PPCallbacks isn't being pushed to give as good of information as Clang can give you, and there may be significant improvements to be made to it which would make life a lot easier for modularize (and other tools, too). E.g. notice that there are some FIXME's on PPCallbacks that might be useful for modularize.</div>
<div style><br></div><div style>On the other hand, keeping track of the SourceRange's in SourceRangeSkipped and ensuring that the same SourceRange's are skipped for a given file might be an easier approach that doesn't require any modifications to clang.</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"><div lang="EN-US" link="blue" vlink="purple"><p class="">
<span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> The fact that it didn’t pass along this flag seems to be an oversight, but I can fix it in a separate
 patch.  Even though the preprocessed output isn’t correct in the case of function macros, because it still incidentally likely turns out different when a macro is used or a macro argument is passed, it’s still valid as a test for comparing the conditions.</span></p>

<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">If you find more style and convention problems, please do pass them along and I will address them before checking in.  But please let me check this in as a
 functional step in the process.  After all, this is still an experimental tool, and I think it would be better to make incremental changes, starting with this as a baseline.</span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Thanks for the reference to clang-format.  It’s made things much easier.</span></p></div></blockquote><div><br></div><div style>
I love clang-format :) I also can't wait for clang-tidy which should similarly automate many of the other stylistic comments that I have made.</div><div style> </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">
<div lang="EN-US" link="blue" vlink="purple">
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">The latest patch is enclosed.</span><span style="color:rgb(31,73,125);font-family:Calibri,sans-serif;font-size:11pt"> </span></p>
</div></blockquote><div><br></div><div style>There are still numerous issues that I mentioned that are unaddressed, from variables still named incorrectly to use of fixed-size char buffers and C-style string manipulation to error-prone memory management. At this point I recommend taking a step back from your current patch and start by taking some iterative steps cleaning up and factoring the existing modularize code as I mentioned above, in preparation for adding new checks.</div>
<div style><br></div><div style>While I don't claim to be a paragon of incremental development, you may find it helpful to see the steps I took leading up to the initial ELF support for yaml2obj, which I think are fairly representative of most new feature work in LLVM: r183332, r183335, r183711</div>
<div style>Notice how the changes to the existing code are tiny and obvious in r183711, and I only added the new functionality after cleaning up the existing functionality to ensure that adding the initial version of my feature was a tiny, incremental step that cleanly meshed with the existing code organization.</div>
<div style>More reading about incremental development: <<a href="http://llvm.org/docs/DeveloperPolicy.html#incremental-changes">http://llvm.org/docs/DeveloperPolicy.html#incremental-changes</a>> (although it talks about "large/invasive changes", I find that the key ideas are fractal)</div>
</div><br></div><div class="gmail_extra" style>-- Sean Silva</div></div>