<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>+// Do macro substitutions in a string.<br></div><div><div>+// FIXME: Note that this is very simple parser, such that it doesn't</div><div>+// support things like function-style macros, macros with string</div>
<div>+// or character literals, and defined().</div><div>+// FIXME: There probably should be a function in the Preprocessor</div><div>+// class that will lex a string, do substitutions, and return either</div><div>+// a token list or a converted string.  However, it's somewhat</div>
<div>+// problematic in that it could interfere with the preprocessor</div><div>+// and lexor state, and call the MacroExpanded handlers again.</div><div>+// So for now, we stick with this simple function, to be extend</div>
<div>+// to support function-style macros.</div><div>+std::string PreprocessorCallbacks::getMacroSubstitutedString(std::string str) {</div><div><br></div><div>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.</div>
</div><div><br></div><div><div><br class="">+  // Return true if this directive matches another.</div><div>+  // (It compares the converted conditionals.)</div><div>+  bool match(PreprocessorDirective *other);</div><div><br>
</div><div>This needs a more useful name. Also, it's unacceptable that this is based on a string comparison; walk clang's macro expansion history at each location and use that to do the comparison.</div><div><br></div>
<div>If I understand your code correctly, what you want to do is basically:</div></div><div><br></div><div style>map<FileAndLine, PreprocessorCondition> M;</div><div style>for each PreprocessorCondition P at FileAndLine FL across the given headers:</div>
<div style>  if (M contains an entry with key FL)</div><div style>    if (M[FL] != P)</div><div style>      diagnoseError(M[FL], P);</div><div style>  M[FL] = P;</div><div style><br></div><div style>Where you have an appropriate equivalence relation defined for PreprocessorCondition which takes into account the entire macro expansion at that location (this is probably what you want to ask about on cfe-dev, but do some research on it first by investigating clang's macro-related diagnostics). This approach is fully general and can easily be expanded to work on arbitrary macro expansions anywhere in the file, as long as an appropriate equivalence relation is available. In reality, you probably want to keep a vector of distinct PreprocessorCondition's (there may be more than just 2 different expansions), and so the real code would look more like:</div>
<div style><br></div><div style><div>map<FileAndLine, vector<PreprocessorCondition>> M;</div><div>for (each PreprocessorCondition P at FileAndLine FL across the given headers)</div><div>  vector<PreprocessorCondition> &V = M[FL];</div>
<div>  if (find(V.begin(), V.end(), C) == V.end())</div><div>    V.push_back(C);</div><div>for (auto &E : M)</div><div>  if (E->second.size() > 1)</div><div>    diagnose(E->second);</div><div><br></div><div style>
The first `for` loop probably won't take exactly that form in the final code, as its body will likely execute inside of a PPCallbacks callback.</div></div><div style><br></div><div style><br></div><div style><br></div>
<div style>Although almost none of your existing code will end up in an updated patch with the algorithm I suggested above, you might find the following comments helpful as you dive into writing the new code (these are just various things I saw while trying to grok your code).</div>
<div style><br></div><div style><div><div>+  HeaderInstanceVectorIterator hiIter = getHeaderInstancesBegin();</div><div>+  HeaderInstanceVectorIterator hiEnd = getHeaderInstancesEnd();</div></div><div><br></div><div>Please also address the naming convention issues *in the patch* as well!</div>
<div><br></div><div>+HeaderFile *PreprocessorCallbacks::getHeaderFile(const std::string& name) {<br></div><div><br></div><div>And the trivial formatting issues! Please, save yourself (and me!) the trouble and just use clang-format.</div>
</div><div><br></div><div>+  HeaderInstance *headerInstance = new HeaderInstance(</div><div>
+  headerFile, topHeader);</div><div><br></div><div>+      delete headerFile;  // No longer needed.</div><div><br></div><div>+  HeaderInstance *newInstance = new HeaderInstance(</div><div>+  headerFile, topHeader);</div>
<div>
<br></div><div><div>+void HeaderTracker::deleteHeaderInstances() {</div><div>+  HeaderInstanceVectorIterator hiIter = getHeaderInstancesBegin();</div><div>+  HeaderInstanceVectorIterator hiEnd = getHeaderInstancesEnd();</div>

<div>+  for (; hiIter != hiEnd; ++hiIter)</div><div>+    delete (*hiIter);</div><div>+  HeaderInstances.clear();</div><div>+}</div></div><div><br></div><div>It's still not clear who owns these and what their lifetime semantics are.</div>

<div><br></div><div><div>+#ifndef HEADER_TRACKER_H</div><div>+#define HEADER_TRACKER_H</div><div><br></div><div>Please add a MODULARIZE_ prefix here and to any other header guards that may be missing it.</div>
<div><br></div><div><div>+/// This class tracks the instances of one particular header.  It stores the</div><div>+/// header name and a vector of HeaderInstanceís.  If all instances</div><div><br></div><div>
I'm seeing a non-ASCII character in HeaderInstance<<HERE>>s. Please clean up any other non-ASCII characters as well.</div><div><br></div><div><div>+std::string getIndent(int level) {</div><div>
+  char buffer[256];</div><div>+  int index = 0;</div><div>+</div><div>+  while (level > 0) {</div><div>+    int size = 4;</div><div>+    while (size-- > 0)</div><div>+      buffer[index++] = ' ';</div><div>

+    level--;</div><div>+  }</div><div>+</div><div>+  buffer[index] = '\0';</div><div>+</div><div>+  return std::string(buffer);</div><div>+}</div><div>+</div><div>+}</div><div><br></div><div>Just do `std::string(level * 4, ' ')`.</div>

<div><br></div><div><div>+bool PreprocessorDirective::match(PreprocessorDirective *other) {</div><div>+  if (ConvertedConditional == other->getConvertedConditional()) {</div><div>+    return true;</div><div>
+  }</div><div>+  return false;</div><div>+}</div><div>+</div><div><br></div><div>Just do `return ConvertedConditional == other->getConvertedConditional();`.</div><div><br></div><div><div>+  }</div><div>
+  else</div><div>+    CurrentHeaderFile = mhf;</div><div><br></div><div>Please use our bracing style. I recommend widespread use of clang-format.</div><div>
<div><br></div><div><div>+  // Preprocessor directive accessors.</div><div>+  PreprocessorDirectiveVector &getDirectives() { return Directives; }</div><div>+  int getDirectiveCount() { return Directives.size(); }</div>

<div>+  PreprocessorDirectiveVectorIterator getDirectivesBegin()</div><div>+    { return Directives.begin(); }</div><div>+  PreprocessorDirectiveVectorIterator getDirectivesEnd()</div><div>+    { return Directives.end(); }</div>

<div>+  PreprocessorDirective &getDirective(int index) { return Directives[index]; }</div><div>+  void addDirective(PreprocessorDirective &directive)</div><div>+    { Directives.push_back(directive); };</div><div>

<br></div><div>These seem to be pointless boilerplate. Just access `Directives` directly.</div><div><br></div><div><div>+  // Source location string accessors.</div><div>+  unsigned getLineNumber() { return LineNumber; }</div>
<div>+  void setLineNumber(unsigned value) { LineNumber = value; };</div><div>+</div><div>+  // Unconverted conditional accessors.</div><div>+  std::string &getUnconvertedConditional()</div><div>+    { return UnconvertedConditional; }</div>
<div>+  void setUnconvertedConditional(std::string value)</div><div>+    { UnconvertedConditional = value; };</div><div>+</div><div>+  // Converted conditional accessors.</div><div>+  std::string &getConvertedConditional() { return ConvertedConditional; }</div>
<div>+  void setConvertedConditional(std::string value)</div><div>+    { ConvertedConditional = value; };</div></div><div><br></div><div style>These also seem like pointless boilerplate. Just make this a lightweight value struct and define it near the class that owns instances of it.</div>
<div style><br></div><div style><div style>+}  // end namespace clang<br></div><div style><br></div><div style>This comment is wrong, and so are others. Please fix all your "end namespace" comments.</div></div>
<div><br></div><div><div>+// Collections for users of this typs.</div><div>+typedef llvm::StringMap<HeaderFile *> HeaderFileMap;</div><div>+typedef HeaderFileMap::iterator HeaderFileMapIterator;</div><div>
<br></div><div>These typedefs don't seem to be buying you much, especially the iterator one. There are similar typedefs in other places. You can probably just remove them with little loss of readability.</div><div>
<br></div><div><div>+  int offset = source.find(':', 2);</div><div>+  if (offset < 0)</div><div><br></div><div>std::string::find returns an unsigned type (usually size_t). On a platform with 32-bit int and 64-bit size_t, does the conversion of size_t(0xFFFFFFFFFFFFFFFFULL) to int result in -1? It's hard to tell whether this code is right or wrong; please rectify that.</div>

<div><br></div><div><div>+      returnValue = false;</div><div>+      break;</div><div>+    }</div><div>+  }</div><div>+  return returnValue;</div><div><br></div><div>just `return false;` instead of `break`ing.</div>
<div><br></div><div><div>+  if (Directives.size() == 0)</div><div><br></div><div>Use .empty()</div><div><br></div><div>+  return Directives[Directives.size() - 1].getLineNumber();</div><div><br>
</div><div>Directives.back().getLineNumber()</div><div><br></div><div><div>+// Get directive spelling.</div><div>+const char *PreprocessorDirective::getDirectiveSpelling(</div><div>+    clang::tok::PPKeywordKind kind) {</div>

<div>+  const char *directiveString;</div><div>+  switch (kind) {</div><div>+    case clang::tok::pp_if:</div><div>+      directiveString = "if";</div><div>+      break;</div><div>+    case clang::tok::pp_elif:</div>

<div>+      directiveString = "elif";</div><div>+      break;</div><div>+    case clang::tok::pp_ifdef:</div><div>+      directiveString = "ifdef";</div><div>+      break;</div><div>+    case clang::tok::pp_ifndef:</div>

<div>+      directiveString = "ifndef";</div><div>+      break;</div><div>+    default:</div><div>+      directiveString = "(unknown)";</div><div>+      break;</div><div>+  }</div><div>+  return directiveString;</div>

<div>+}</div><div><br></div><div>Use early returns here. I.e. `case clang::tok::pp_elif: return "elif";`</div></div></div></div></div></div></div></div></div></div></div></div></div><div><br></div><div><div>+  HeaderInstanceVectorIterator hiIter = getHeaderInstancesBegin();</div>
<div>+  HeaderInstanceVectorIterator hiEnd = getHeaderInstancesEnd();</div><div>+  int directiveCount = 0x7fffffff;  // Big number.</div><div>+  // Get the number of instances which is least.</div><div>+  for (; hiIter != hiEnd; ++hiIter) {</div>
<div>+    HeaderInstance *headerInstance = (*hiIter);</div><div>+    HeaderFile *headerFile = headerInstance->getHeaderFile();</div><div>+    if (headerFile->getDirectiveCount() < directiveCount)</div><div>+      directiveCount = headerFile->getDirectiveCount();</div>
<div>+  }</div></div><div><br></div><div style>This is needlessly verbose. The following seems a lot more idiomatic and clear to me:</div><div style><br></div><div style><div><div>typedef std::vector<HeaderInstance *>::iterator Iter;</div>
<div>size_t MinDirectives = std::numeric_limits<size_t>::max();</div><div>for (Iter I = HeaderInstances.begin(), E = HeaderInstances.end(); I != E; ++I)</div><div>  MinDirectives = std::min((*I)->getHeaderFile->getDirectiveCount(),</div>
<div>                            MinDirectives);</div></div><div><br></div></div><div style>And it's still not clear who owns all these pointers.</div><div style><br></div><div style><div>+// Report instances where header snapshots differ.</div>
<div>+// Returns true if no errors, i.e. no differing snapshots.</div><div>+bool HeaderTracker::report() {</div><div><br></div><div style>You really need to clarify what the "algorithm" being done here is.</div>
<div><br></div><div><div>+    for (int headerInstanceIndex = 1;</div><div>+        headerInstanceIndex < headerInstanceCount;</div><div>+        headerInstanceIndex++) {</div></div><div><br></div><div style>Why does this loop start at `1`? Also, use shorter loop variable names (I, E, J, JE, etc.)</div>
<div style><br></div><div style><div>+    // We had one or more mismatches.  Output diagnostic and exit.</div><div>+    errs() << "warning: The instances of header "</div><div>+      << Name << " have different contents after preprocessing:\n";</div>
<div>+    for (int headerInstanceIndex = 0;</div><div>+        headerInstanceIndex < headerInstanceCount;</div><div>+        headerInstanceIndex++) {</div><div>+      HeaderInstance *headerInstance =</div><div>+        getHeaderInstance(headerInstanceIndex);</div>
<div>+      HeaderFile *headerFile = headerInstance->getHeaderFile();</div><div>+      errs() << getIndent(1)</div><div>+        << "When included or nested in these top-level headers:\n";</div><div>
+      TopHeaderVectorIterator thIter =</div><div>+        headerInstance->getTopHeadersBegin();</div><div>+      TopHeaderVectorIterator thEnd = headerInstance->getTopHeadersEnd();</div><div>+      for (; thIter != thEnd; ++thIter) {</div>
<div>+        errs() << getIndent(2) << *thIter << "\n";</div><div>+      }</div><div>+      errs() << getIndent(1)</div><div>+        << "This preprocessor directive is mismatched (the parentheses"</div>
<div>+          " show the condition after macro substitution):\n";</div><div>+      PreprocessorDirective *directive =</div><div>+        &headerFile->getDirective(directiveIndex);</div><div>+      directive->dump(2, Name.c_str());</div>
<div><br></div><div style>Factor out this code which produces the diagnostic into a method.</div><div style><br></div><div style><div>+    break;  // Break out now that we've found directives that don't match,</div>
<div>+            // as the directives list might not match anymore.</div><div>+  }</div><div>+  return false;   // There was a mismatch.</div><div><br></div><div style>Use an early return here. Also, it looks like this function returns `false` regardless of whether a mismatch was found.</div>
</div></div></div><div><br></div><div>
-- Sean Silva</div><div> </div></div></div></div>