<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;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;}
@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">Thanks a bunch, 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">I’ve cleaned up the sources a lot per your comments, but I still need to replace the macro substitution function, which will mean I can do away with the symbol
storing too. I couldn’t find a function such as I need, i.e. a function in Preprocessor that will either take a string or source range as input and produce either a string or token vector, with macro substitutions done using the current preprocessor state,
all without adversely affecting the preprocessor state. But it’s a huge class, so I might be missing it. Do you or anyone know of such a function? Otherwise, I’ll work on a separate patch to add one to Preprocessor.<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">Also, in addition to the macro substitution, I think I can relatively easily add a similar message to the “header (file) has different contents depending on
how it was included” error, relating the error to the differing preprocessor conditional. Likewise, an option for showing the include hierarchy would help.<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">Anyway, may I check this in as a working intermediate step, in case folks find it useful?<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">Thanks.<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> Tuesday, June 11, 2013 1:53 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">Hi John, here are some mostly stylistic comments:<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+void ModularizeHeaderTracker::AddHeaderFile(<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *headerFile, std::string topHeader) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderInstanceVectorIterator iter = HeaderInstances.begin();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderInstanceVectorIterator EI = HeaderInstances.end();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ for (; iter != EI; ++iter) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *existingHeader = (*iter)->GetHeaderFile();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (existingHeader->Match(headerFile)) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ (*iter)->AddTopHeader(topHeader);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ return;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ }<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please follow our naming conventios <<a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a>>
and indentation conventions (2 space soft tabs; you may also find clang-format useful).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+#if _MSC_VER >= 1400 // VC 8.0 and later deprecate snprintf and _snprintf.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+# define snprintf _snprintf_s<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+#elif _MSC_VER<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+# define snprintf _snprintf<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+#endif<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Avoid preprocessor conditionals except in libSupport. You shouldn't be using this function anyway, really (see below).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+std::string ModularizePPDirective::PrintToString(<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ int level, std::string fileName) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ char buffer[1024];<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ char lineBuffer[80];<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ snprintf(lineBuffer, sizeof(lineBuffer) - 1, ":%d:1", LineNumber);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ strncpy(buffer, Indent(level).c_str(), sizeof(buffer) - 1);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ strncat(buffer, fileName.c_str(), sizeof(buffer) - 1);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ strncat(buffer, lineBuffer, sizeof(buffer) - 1);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ strncat(buffer, ": ", sizeof(buffer) - 1);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Use raw_ostream for this instead of error-prone C-style string manipulation. raw_svector_ostream or raw_string_ostream are a good fit for this.<o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+// Do macro substitutions in a string.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// FIXME: Note that this is very simple parser, such that it doesn't<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// support things like function-style macros, macros with string<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// or character literals, and defined().<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// FIXME: There probably should be a function in the Preprocessor<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// class that will lex a string, do substitutions, and return either<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+// a token list or a converted string.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+std::string ModularizePPCallbacks::DoMacroSubstitutions(std::string str) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I'm pretty sure that Clang has functionality to do this. Please don't invent your own. If you need to add functionality to clang's Preprocessor, please do so in a separate patch.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+#if MPPC_TRACE<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ Trace(Loc, "Ifndef(%s, %s) called.\n",<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ macroName.c_str(), (MD ? " (defined)" : " (not defined)"));<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ Trace("CurrentHeaderFile.Name = %s\n", CurrentHeaderFile->GetName().c_str());<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+#endif<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">We have the DEBUG macro (and friends) which can use used for this purpose.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+ /// \brief Hook called whenever an \#elif is seen.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ /// \param Loc the source location of the directive.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ /// \param ConditionRange The SourceRange of the expression being tested.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ /// \param IfLoc the source location of the \#if/\#ifdef/\#ifndef directive.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ // FIXME: better to pass in a list (or tree!) of Tokens.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ void Elif(SourceLocation Loc, SourceRange ConditionRange,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ SourceLocation IfLoc);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please don't just copy the documentation from PPCallbacks. You may want to discuss what this particular concrete subclass is doing in the callbacks though.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">+#if 0 // Can result in mismatched files.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ ModularizeHeaderFile *mhf = NULL;<o:p></o:p></p>
</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">+ AddHeaderFile(mhf);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ CurrentHeaderFile = mhf;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (!RootHeaderFile)<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">+ else<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ CurrentHeaderFile = mhf;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+#endif<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Don't leave in disabled code.<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal">-- Sean Silva<o:p></o:p></p>
</div>
</div>
</div>
</div>
</body>
</html>