<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Sean,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Here’s another shot at it that I believe addresses all issues, except for the following:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I didn’t get a lead on a function to replace getSourceSnippet, but with your suggestion about StringRef, and Eli’s pointer to getCharacterData, I rewrote it
to be much simpler.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I didn’t get any leads pertaining to getMacroSubstitutedString, so no changes there yet.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I addressed the naming convention changes in the old Modularize.cpp in a separate check-in.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I can’t really break it up any smaller in a meaningful way.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">-John<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Sean Silva [mailto:silvas@purdue.edu]
<br>
<b>Sent:</b> Monday, June 17, 2013 4:11 PM<br>
<b>To:</b> Thompson, John<br>
<b>Cc:</b> cfe-commits@cs.uiuc.edu; John.Thompson.JTSoftware@gmail.com<br>
<b>Subject:</b> Re: PLEASE REVIEW: modularize: new preprocessor conditional directive checking feature<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Mon, Jun 17, 2013 at 1:15 PM, Thompson, John <<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>> wrote:<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Sean,</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks again for your help.</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Sorry, I overlooked revising the function that was doing some old C-style string handling. I’ve
addressed that now (in the new ModularizePPDirective::print function).</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The only other item I didn’t address was the macro substitutions function, which was intentional
as I described. I’ve looked into this further, but I’d still like to punt on it for now. Because there isn’t such a function already, it appears the only clean way to do it would be to instantiate a new Lexer and Preprocessor object, linking the new preprocessor
to the old so it can find the macros (there seems to be a pointer for this). It would also require adding at least a new Lexer constructor, so it can set up the buffer pointers and correct state. The constructor for lexing pragmas is close, but it sets the
lexer in raw mode, which I don’t want. There’s also the problem that it will call the MacroExpanded handler again, and I also worry there might be other side-effects due to the preprocessor and lexer states being changed inadvertently. Since this is an external
tool, I’m kind of thinking that it would be simpler to just use the function I put in for now, and I will look into it some more when I try to figure out how to do the function-style macro expansion, possibly using the existing code in the Preprocessor class.</span><o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please ask on cfe-dev about what you are trying to achieve and how best to achieve it. While this specific functionality (a function that does this) may not be available, clang may have facilities for producing the end result in a simpler
way. Since clang already gives detailed information about macro expansion for many diagnostics, it seems likely that your goals here can be achieved with the current code.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Regarding breaking the patch up, it might be easier to consider the changes in three parts:</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">1.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The new classes.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">2.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The changes to Modularize.cpp.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">3.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The changes to the tests.</span> <o:p></o:p></p>
</div>
</div>
</blockquote>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’ll provide these in separate patch files, though the associated changes will need to go in at the same time, however.</span><o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal">In the LLVM community "breaking the patch up" is understood to mean breaking into a series of patches that can be applied in series and that compile and run (and pass all tests) after each patch is applied. Try to keep changes that need
to be applied together together in a single patch file (this includes tests); this is basically always achievable (the exception is typically changes that affect multiple projects (e.g. changes to LLVM that require changes to clang), but that isn't an issue
in this situation).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The new classes (mod_2013_06_17_new_patch.txt). I should give some description of the new classes
added and how they work together:</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">1.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizePPCallbacks – Derives from the Clang PPCallbacks class to track preprocessor actions, such as changing files and handling preprocessor directives and macro expansions.
It has to figure out when a new header file is entered and left, as the provided handler is not particularly clear about it. It also stores a map of macro expansions obtained from the MacroExpands callback, for use by a function that effectively preprocesses
a conditional. It handles the top-level aspects of collecting header file instance information, and tracking the preprocessor conditional directives.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">2.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizePPDirective – Stores information about one preprocessor directive instance, presently limited to #if, #elif, #ifdef, and #ifndef, since that is all modularize needs
for now. It stores the source file line number, a directive kind code, and both the unpreprocessed and preprocessed conditional source code snippet.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">3.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizeHeaderFile – Store a header file name and a vector of ModularizePPDirective instances collected for that header file.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">4.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizeHeaderInstance – Stores a pointer to a ModularizeHeaderFile for a header, and a vector of header file names for the headers from the modularize header list that
reference the particular header, either directly or indirectly via some nested include. If separate instances of the header are encountered when modularize processes its header list, if the preprocessed directive conditionals stored in the ModularizePPDirective
vector are the same for and existing ModularizeHeaderFile object, the top-level header name is added to the instance, effectively reusing the ModularizeHeaderFile object. If a header is seen for the first time, or if the preprocessed conditionals for the
stored directives don’t match those of an instance of the header seen before, a new ModularizeHeaderInstance object is created and saved.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">5.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizeHeaderTracker – Tracks the instances of one particular header. It stores the header name and a vector of ModularizeHeaderInstance’s. If all instances of a header
seen have the same conditionals after preprocessing, there will only be one ModularizeHeaderInstance. If one or more conditionals were difference, there will be two or more instance objects saved.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">6.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">ModularizeMasterHeaderTracker – Stores a map of all the ModularizeHeaderTracker objects, and provides an “addHeaderFile” function for handling a header file, and a “report”
function for outputting the warnings about the preprocessor conditional directive mismatches.</span><o:p></o:p></p>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please put these descriptions in doxygen comments for their respective classes.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The changes to Modularize.cpp (mod_2013_06_17_modularize_patch.txt):</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">1.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Add an option for disabling the preprocessing consistency checking. This is a fallback, in case of problems with the mechanism, or to reduce warnings volume.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">2.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Set up a ModularizePPCallbacks object for tracking the preprocessor. This is done in the CollectEntitiesConsumer object.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">3.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Set up a ModularizeMasterHeaderTracker for storing the header instance data. This is done in the CollectEntitiesConsumer object.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">4.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Call the ModularizeMasterHeaderTracker::report function to report any warnings about the preprocessor conditional directive mismatches.</span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">5.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Fixed some naming convention issues.</span><o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">At the very least number 5 can be a separate patch (feel free to commit it directly).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The changes to the tests (mod_2013_06_17_test_patch.txt):</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">1.</span><span style="font-size:7.0pt;color:#1F497D">
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Some new lines in a couple of files for the new feature.</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’m also including a zip with the changed files.</span><o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal">For future reference, including a zip like this is not necessary (nor particularly desirable for most people's workflow I assume).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’m hoping I can check this in soon, as it makes me nervous to sit on so much, and makes it harder
to continue experimenting. Since this is still an experimental tool, I’m hoping we can improve it in incremental steps.</span><o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal">At the very least, the stylistic issues (variable naming, line endings, tabs, formatting, etc.) will need to be cleared up before committing.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">You mentioned you have other suggestions too. Please do feel free to send them.</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">One thing I’m aware of is that the collections are probably leaking the objects’ memory. I can
fix that if necessary.</span><o:p></o:p></p>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Yes, please fix that; memory leaks are not acceptable.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Some other things:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">* Please rename things called `ModularizeFoo` to be just `Foo` if they are already in the `Modularize` namespace.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">* Please name your variables in accordance with LLVM naming conventions and put `*` and `&` on the right.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">* Remove hard tabs.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">* Use Unix (LF) line endings throughout (I see many CRLF) and make sure to have <=80 columns per line.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+bool ModularizeHeaderTracker::report() {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ int headerInstanceIndex;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ int headerInstanceCount = getHeaderInstanceCount();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderInstance *headerInstance;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderInstance *headerInstance0;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *headerFile;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *headerFile0;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ int directiveIndex;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizePPDirective *directive;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizePPDirective *directive0;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ bool mismatch;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please move these declarations to their point of first use. Also clarify what the `0` suffix on some of these names means (possibly by renaming to a more useful name).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (getHeaderInstanceCount() > 1) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ int directiveCount = 0x7fffffff; // Big number.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderInstanceVectorIterator hiIter = getHeaderInstancesBegin();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Reverse the condition of this `if` and use an early exit to simplify and reduce indentation <<a href="http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code">http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code</a>>.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ if (mismatch) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ errs() << "warning: The instances of header " << Name << " have different contents after preprocessing:\n";<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Same for this `if`.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+// Represents a preprocessor directive kind.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+enum ModularizePPDirectiveKind {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ MPPD_If,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ MPPD_ElIf,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ MPPD_IfDef,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ MPPD_IfNDef<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+};<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Surely clang already has an enum that can serve this purpose?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal">+// Get directive spelling.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+const char *ModularizePPDirective::getDirectiveSpelling(<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizePPDirectiveKind kind) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ const char *directiveString;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ switch (kind) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ case MPPD_If:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ directiveString = "if";<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Surely clang already has something that can do this?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *mhf = new ModularizeHeaderFile(topFile.str(), PP);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ addHeaderFile(mhf);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ CurrentHeaderFile = mhf;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ RootHeaderFile = mhf;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Who owns this? When is it freed?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ mhf = getHeaderFile(*fileName);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (mhf == NULL) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ mhf = new ModularizeHeaderFile(*fileName, PP);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Same here.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+// Retrieve source snippet from file image.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+std::string ModularizePPCallbacks::getSourceSnippet(SourceRange sourceRange) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Surely clang has this functionality somewhere already?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ std::string returnValue(bPtr, length);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ return returnValue;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">You can directly `return std::string(...)`.<o:p></o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ // Trim snippet.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ while ((*bPtr <= ' ') && (length != 0)) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ bPtr++;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ length--;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ }<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ while ((length != 0) && (bPtr[length - 1] <= ' '))<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ length--;<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">StringRef::trim?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ using namespace clang;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ using namespace llvm;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">No `using namespace` in headers. (Second sentence of <<a href="http://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std">http://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std</a>>)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+ return (*iter).second;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Use operator-> (e.g. iter->second), here and in other places.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+#ifndef MODULARIZEHEADERTRACKER_H<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+#define MODULARIZEHEADERTRACKER_H<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">In LLVM we typically put underscores between "words" in header guard macros. So e.g. this should be MODULARIZE_HEADER_TRACKER_H.<o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">-- Sean Silva<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>