On Mon, Jul 29, 2013 at 12:07 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><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: jtsoftware<br>
Date: Mon Jul 29 14:07:00 2013<br>
New Revision: 187370<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=187370&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=187370&view=rev</a><br>
Log:<br>
Fixed comment issues (non-ASCII chars, typos) per review, expanded some comments.<br>
<br>
Modified:<br>
clang-tools-extra/trunk/modularize/Modularize.cpp<br>
clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp<br>
<br>
Modified: clang-tools-extra/trunk/modularize/Modularize.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/Modularize.cpp?rev=187370&r1=187369&r2=187370&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/Modularize.cpp?rev=187370&r1=187369&r2=187370&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/modularize/Modularize.cpp (original)<br>
+++ clang-tools-extra/trunk/modularize/Modularize.cpp Mon Jul 29 14:07:00 2013<br>
@@ -35,17 +35,43 @@<br>
// Modularize will do normal parsing, reporting normal errors and warnings,<br>
// but will also report special error messages like the following:<br>
//<br>
-// error: '(symbol)' defined at multiple locations:<br>
-// (file):(row):(column)<br>
-// (file):(row):(column)<br>
+// error: '(symbol)' defined at multiple locations:<br>
+// (file):(row):(column)<br>
+// (file):(row):(column)<br>
//<br>
-// error: header '(file)' has different contents dependening on how it was<br>
-// included<br>
+// error: header '(file)' has different contents dependening on how it was<br>
+// included<br></blockquote><div><br></div><div>Dependening?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
//<br>
// The latter might be followed by messages like the following:<br>
//<br>
-// note: '(symbol)' in (file) at (row):(column) not always provided<br>
+// note: '(symbol)' in (file) at (row):(column) not always provided<br>
//<br>
+// Checks will also be performed for macro expansions, defined(macro)<br>
+// expressions, and preprocessor conditional directives that evaluate<br>
+// inconsistently, and can produce error messages like the following:<br>
+//<br>
+// (...)/SubHeader.h:11:5:<br>
+// #if SYMBOL == 1<br>
+// ^<br>
+// error: Macro instance 'SYMBOL' has different values in this header,<br>
+// depending on how it was included.<br>
+// 'SYMBOL' expanded to: '1' with respect to these inclusion paths:<br>
+// (...)/Header1.h<br>
+// (...)/SubHeader.h<br>
+// (...)/SubHeader.h:3:9:<br>
+// #define SYMBOL 1<br>
+// ^<br>
+// Macro defined here.<br>
+// 'SYMBOL' expanded to: '2' with respect to these inclusion paths:<br>
+// (...)/Header2.h<br>
+// (...)/SubHeader.h<br>
+// (...)/SubHeader.h:7:9:<br>
+// #define SYMBOL 2<br>
+// ^<br>
+// Macro defined here.<br>
+//<br>
+// See PreprocessorTracker.cpp for additional details.<br>
+//<br>
// Future directions:<br>
//<br>
// Basically, we want to add new checks for whatever we can check with respect<br>
@@ -54,7 +80,7 @@<br>
// Some ideas:<br>
//<br>
// 1. Try to figure out the preprocessor conditional directives that<br>
-// contribute to problems.<br>
+// contribute to problems and tie them to the inconsistent definitions.<br>
//<br>
// 2. Check for correct and consistent usage of extern "C" {} and other<br>
// directives. Warn about #include inside extern "C" {}.<br>
<br>
Modified: clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp?rev=187370&r1=187369&r2=187370&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp?rev=187370&r1=187369&r2=187370&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp (original)<br>
+++ clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp Mon Jul 29 14:07:00 2013<br>
@@ -1,37 +1,60 @@<br>
-//=- PreprocessorTracker.cpp - Preprocessor tracking -*- C++ -*-=//<br>
+//===--- PreprocessorTracker.cpp - Preprocessor tracking -*- C++ -*------===//<br>
//<br>
// The LLVM Compiler Infrastructure<br>
//<br>
// This file is distributed under the University of Illinois Open Source<br>
// License. See LICENSE.TXT for details.<br>
//<br>
+//===--------------------------------------------------------------------===//<br>
+//<br>
// The Basic Idea<br>
//<br>
// Basically we install a PPCallbacks-derived object to track preprocessor<br>
// activity, namely when a header file is entered/exited, when a macro<br>
-// is expanded, when “defined” is used, and when #if, #elif, #ifdef,<br>
-// and #ifndef are used. We save the state of macro and “defined”<br>
+// is expanded, when "defined" is used, and when #if, #elif, #ifdef,<br>
+// and #ifndef are used. We save the state of macro and "defined"<br>
// expressions in a map, keyed on a name/file/line/column quadruple.<br>
-// The map entries store the different states (values) a macro expansion,<br>
-// “defined” expression, or condition expression has in the course of<br>
+// The map entries store the different states (values) that a macro expansion,<br>
+// "defined" expression, or condition expression has in the course of<br>
// processing for the one location in the one header containing it,<br>
// plus a list of the nested include stacks for the states. When a macro<br>
-// or “defined” expression evaluates to the same value, which is the<br>
+// or "defined" expression evaluates to the same value, which is the<br>
// desired case, only one state is stored. Similarly, for conditional<br>
// directives, we save the condition expression states in a separate map.<br>
//<br>
// This information is collected as modularize compiles all the headers<br>
// given to it to process. After all the compilations are performed,<br>
-// a check is performed for any entries in the map that contain more<br>
-// than one different state, and an output message is generated, such<br>
-// as the one shown previously.<br>
+// a check is performed for any entries in the maps that contain more<br>
+// than one different state, and for these an output message is generated.<br>
+//<br>
+// For example:<br>
+//<br>
+// (...)/SubHeader.h:11:5:<br>
+// #if SYMBOL == 1<br>
+// ^<br>
+// error: Macro instance 'SYMBOL' has different values in this header,<br>
+// depending on how it was included.<br>
+// 'SYMBOL' expanded to: '1' with respect to these inclusion paths:<br>
+// (...)/Header1.h<br>
+// (...)/SubHeader.h<br>
+// (...)/SubHeader.h:3:9:<br>
+// #define SYMBOL 1<br>
+// ^<br>
+// Macro defined here.<br>
+// 'SYMBOL' expanded to: '2' with respect to these inclusion paths:<br>
+// (...)/Header2.h<br>
+// (...)/SubHeader.h<br>
+// (...)/SubHeader.h:7:9:<br>
+// #define SYMBOL 2<br>
+// ^<br>
+// Macro defined here.<br>
//<br>
// Design and Implementation Details<br>
//<br>
// A PreprocessorTrackerImpl class implements the PreprocessorTracker<br>
// interface. It uses a PreprocessorCallbacks class derived from PPCallbacks<br>
// to track preprocessor activity, namely entering/exiting a header, macro<br>
-// expansions, use of “defined” expressions, and #if, #elif, #ifdef, and<br>
+// expansions, use of "defined" expressions, and #if, #elif, #ifdef, and<br>
// #ifndef conditional directives. PreprocessorTrackerImpl stores a map<br>
// of MacroExpansionTracker objects keyed on a name/file/line/column<br>
// value represented by a light-weight PPItemKey value object. This<br>
@@ -41,7 +64,7 @@<br>
// directives.<br>
//<br>
// The MacroExpansionTracker object represents one macro reference or use<br>
-// of a “defined” expression in a header file. It stores a handle to a<br>
+// of a "defined" expression in a header file. It stores a handle to a<br>
// string representing the unexpanded macro instance, a handle to a string<br>
// representing the unpreprocessed source line containing the unexpanded<br>
// macro instance, and a vector of one or more MacroExpansionInstance<br>
@@ -65,7 +88,7 @@<br>
// a different place, a new MacroExpansionInstance object representing<br>
// that case will be added to the vector in MacroExpansionTracker. If a<br>
// macro instance expands to a value already seen before, the<br>
-// InclusionPathHandle representing that case’s include file hierarchy<br>
+// InclusionPathHandle representing that case's include file hierarchy<br>
// will be added to the existing MacroExpansionInstance object.<br>
<br>
// For checking conditional directives, the ConditionalTracker class<br>
@@ -83,18 +106,18 @@<br>
// to the strings.<br>
//<br>
// PreprocessorTrackerImpl also maintains a list representing the unique<br>
-// headers, which is just a vector of StringHandles for the header file<br>
+// headers, which is just a vector of StringHandle's for the header file<br>
// paths. A HeaderHandle abstracts a reference to a header, and is simply<br>
// the index of the stored header file path.<br>
//<br>
-// A HeaderInclusionPath class abstract a unique hierarchy of header file<br>
+// A HeaderInclusionPath class abstracts a unique hierarchy of header file<br>
// inclusions. It simply stores a vector of HeaderHandles ordered from the<br>
// top-most header (the one from the header list passed to modularize) down<br>
// to the header containing the macro reference. PreprocessorTrackerImpl<br>
// stores a vector of these objects. An InclusionPathHandle typedef<br>
// abstracts a reference to one of the HeaderInclusionPath objects, and is<br>
// simply the index of the stored HeaderInclusionPath object. The<br>
-// MacroExpansionInstance object stores a vector of these handle so that<br>
+// MacroExpansionInstance object stores a vector of these handles so that<br>
// the reporting function can display the include hierarchies for the macro<br>
// expansion instances represented by that object, to help the user<br>
// understand how the header was included. (A future enhancement might<br>
@@ -109,15 +132,15 @@<br>
// through the compilation. A PreprocessorTracker instance is created and<br>
// passed down to the AST action and consumer objects in modularize. For<br>
// each new compilation instance, the consumer calls the<br>
-// PreprocessorTracker’s handleNewPreprocessorEntry function, which sets<br>
+// PreprocessorTracker's handleNewPreprocessorEntry function, which sets<br>
// up a PreprocessorCallbacks object for the preprocessor. At the end of<br>
-// the compilation instance, the PreprocessorTracker’s<br>
+// the compilation instance, the PreprocessorTracker's<br>
// handleNewPreprocessorExit function handles cleaning up with respect<br>
// to the preprocessing instance.<br>
//<br>
// The PreprocessorCallbacks object uses an overidden FileChanged callback<br>
// to determine when a header is entered and exited (including exiting the<br>
-// header during #include directives). It calls PreprocessorTracker’s<br>
+// header during #include directives). It calls PreprocessorTracker's<br>
// handleHeaderEntry and handleHeaderExit functions upon entering and<br>
// exiting a header. These functions manage a stack of header handles<br>
// representing by a vector, pushing and popping header handles as headers<br>
@@ -127,14 +150,14 @@<br>
// The PreprocessorCallbacks object uses an overridden MacroExpands callback<br>
// to track when a macro expansion is performed. It calls a couple of helper<br>
// functions to get the unexpanded and expanded macro values as strings, but<br>
-// then calls PreprocessorTrackerImpl’s addMacroExpansionInstance function to<br>
+// then calls PreprocessorTrackerImpl's addMacroExpansionInstance function to<br>
// do the rest of the work. The getMacroExpandedString function uses the<br>
-// preprocessor’s getSpelling to convert tokens to strings using the<br>
+// preprocessor's getSpelling to convert tokens to strings using the<br>
// information passed to the MacroExpands callback, and simply concatenates<br>
// them. It makes recursive calls to itself to handle nested macro<br>
// definitions, and also handles function-style macros.<br>
//<br>
-// PreprocessorTrackerImpl’s addMacroExpansionInstance function looks for<br>
+// PreprocessorTrackerImpl's addMacroExpansionInstance function looks for<br>
// an existing MacroExpansionTracker entry in its map of MacroExampleTracker<br>
// objects. If none exists, it adds one with one MacroExpansionInstance and<br>
// returns. If a MacroExpansionTracker object already exists, it looks for<br>
@@ -148,14 +171,14 @@<br>
// the macro reference and the macro definition stored as string handles.<br>
// These helper functions use the current source manager from the<br>
// preprocessor. This is done in advance at this point in time because the<br>
-// source manager doesn’t exist at the time of the reporting.<br>
+// source manager doesn't exist at the time of the reporting.<br>
//<br>
// For conditional check, the PreprocessorCallbacks class overrides the<br>
// PPCallbacks handlers for #if, #elif, #ifdef, and #ifndef. These handlers<br>
// call the addConditionalExpansionInstance method of<br>
// PreprocessorTrackerImpl. The process is similar to that of macros, but<br>
// with some different data and error messages. A lookup is performed for<br>
-// the conditional, and if a ConditionalTracker object doesn’t yet exist for<br>
+// the conditional, and if a ConditionalTracker object doesn't yet exist for<br>
// the conditional, a new one is added, including adding a<br>
// ConditionalExpansionInstance object to it to represent the condition<br>
// expression state. If a ConditionalTracker for the conditional does<br>
@@ -169,16 +192,16 @@<br>
//<br>
// After modularize has performed all the compilations, it enters a phase<br>
// of error reporting. This new feature adds to this reporting phase calls<br>
-// to the PreprocessorTracker’s reportInconsistentMacros and<br>
+// to the PreprocessorTracker's reportInconsistentMacros and<br>
// reportInconsistentConditionals functions. These functions walk the maps<br>
-// of MacroExpansionTracker’s and ConditionalTracker’s respectively. If<br>
+// of MacroExpansionTracker's and ConditionalTracker's respectively. If<br>
// any of these objects have more than one MacroExpansionInstance or<br>
// ConditionalExpansionInstance objects, it formats and outputs an error<br>
// message like the example shown previously, using the stored data.<br>
//<br>
// A potential issue is that there is some overlap between the #if/#elif<br>
// conditional and macro reporting. I could disable the #if and #elif,<br>
-// leaving just the #ifdef and #ifndef, since these don’t overlap. Or,<br>
+// leaving just the #ifdef and #ifndef, since these don't overlap. Or,<br>
// to make clearer the separate reporting phases, I could add an output<br>
// message marking the phases.<br>
//<br>
@@ -531,7 +554,7 @@ public:<br>
//<br>
// This class represents an instance of a macro expansion with a<br>
// unique value. It also stores the unique header inclusion paths<br>
-// for use in telling the user the nested include path f<br>
+// for use in telling the user the nested include path to the header.<br>
class MacroExpansionInstance {<br>
public:<br>
MacroExpansionInstance(StringHandle MacroExpanded,<br>
@@ -577,7 +600,7 @@ public:<br>
// This class represents one macro expansion, keyed by a PPItemKey.<br>
// It stores a string representing the macro reference in the source,<br>
// and a list of ConditionalExpansionInstances objects representing<br>
-// the unique value the condition expands to in instances of the header.<br>
+// the unique values the condition expands to in instances of the header.<br>
class MacroExpansionTracker {<br>
public:<br>
MacroExpansionTracker(StringHandle MacroUnexpanded,<br>
@@ -634,9 +657,9 @@ public:<br>
<br>
// Conditional expansion instance.<br>
//<br>
-// This class represents an instance of a macro expansion with a<br>
-// unique value. It also stores the unique header inclusion paths<br>
-// for use in telling the user the nested include path f<br>
+// This class represents an instance of a condition exoression result<br>
+// with a unique value. It also stores the unique header inclusion paths<br>
+// for use in telling the user the nested include path to the header.<br>
class ConditionalExpansionInstance {<br>
public:<br>
ConditionalExpansionInstance(bool ConditionValue, InclusionPathHandle H)<br>
@@ -673,8 +696,9 @@ public:<br>
//<br>
// This class represents one conditional directive, keyed by a PPItemKey.<br>
// It stores a string representing the macro reference in the source,<br>
-// and a list of MacroExpansionInstance objects representing<br>
-// the unique value the macro expands to in instances of the header.<br>
+// and a list of ConditionExpansionInstance objects representing<br>
+// the unique value the condition expression expands to in instances of<br>
+// the header.<br>
class ConditionalTracker {<br>
public:<br>
ConditionalTracker(clang::tok::PPKeywordKind DirectiveKind,<br>
@@ -927,6 +951,7 @@ public:<br>
PPItemKey InstanceKey(PP, MacroName, H, InstanceLoc);<br>
PPItemKey DefinitionKey(PP, MacroName, H, DefinitionLoc);<br>
MacroExpansionMapIter I = MacroExpansions.find(InstanceKey);<br>
+ // If existing instance of expansion not found, add one.<br>
if (I == MacroExpansions.end()) {<br>
std::string InstanceSourceLine =<br>
getSourceLocationString(PP, InstanceLoc) + ":\n" +<br>
@@ -939,13 +964,17 @@ public:<br>
addString(InstanceSourceLine), DefinitionKey,<br>
addString(DefinitionSourceLine), InclusionPathHandle);<br>
} else {<br>
+ // We've seen the macro before. Get its tracker.<br>
MacroExpansionTracker &CondTracker = I->second;<br>
+ // Look up an existing instance value for the macro.<br>
MacroExpansionInstance *MacroInfo =<br>
CondTracker.findMacroExpansionInstance(addString(MacroExpanded),<br>
DefinitionKey);<br>
+ // If found, just add the inclusion path to the instance.<br>
if (MacroInfo != NULL)<br>
MacroInfo->addInclusionPathHandle(InclusionPathHandle);<br>
else {<br>
+ // Otherwise add a new instance with the unique value.<br>
std::string DefinitionSourceLine =<br>
getSourceLocationString(PP, DefinitionLoc) + ":\n" +<br>
getSourceLine(PP, DefinitionLoc) + "\n";<br>
@@ -967,6 +996,7 @@ public:<br>
StringHandle ConditionUnexpandedHandle(addString(ConditionUnexpanded));<br>
PPItemKey InstanceKey(PP, ConditionUnexpandedHandle, H, InstanceLoc);<br>
ConditionalExpansionMapIter I = ConditionalExpansions.find(InstanceKey);<br>
+ // If existing instance of condition not found, add one.<br>
if (I == ConditionalExpansions.end()) {<br>
std::string InstanceSourceLine =<br>
getSourceLocationString(PP, InstanceLoc) + ":\n" +<br>
@@ -975,12 +1005,16 @@ public:<br>
ConditionalTracker(DirectiveKind, ConditionValue, ConditionUnexpandedHandle,<br>
InclusionPathHandle);<br>
} else {<br>
+ // We've seen the conditional before. Get its tracker.<br>
ConditionalTracker &CondTracker = I->second;<br>
+ // Look up an existing instance value for the condition.<br>
ConditionalExpansionInstance *MacroInfo =<br>
CondTracker.findConditionalExpansionInstance(ConditionValue);<br>
+ // If found, just add the inclusion path to the instance.<br>
if (MacroInfo != NULL)<br>
MacroInfo->addInclusionPathHandle(InclusionPathHandle);<br>
else {<br>
+ // Otherwise add a new instance with the unique value.<br>
CondTracker.addConditionalExpansionInstance(ConditionValue,<br>
InclusionPathHandle);<br>
}<br>
@@ -991,20 +1025,25 @@ public:<br>
// Returns true if any mismatches.<br>
bool reportInconsistentMacros(llvm::raw_ostream &OS) {<br>
bool ReturnValue = false;<br>
+ // Walk all the macro expansion trackers in the map.<br>
for (MacroExpansionMapIter I = MacroExpansions.begin(),<br>
E = MacroExpansions.end();<br>
I != E; ++I) {<br>
const PPItemKey &ItemKey = I->first;<br>
MacroExpansionTracker &MacroExpTracker = I->second;<br>
+ // If no mismatch (only one instance value) continue.<br>
if (!MacroExpTracker.hasMismatch())<br>
continue;<br>
+ // Tell caller we found one or more errors.<br>
ReturnValue = true;<br>
+ // Start the error message.<br>
OS << *MacroExpTracker.InstanceSourceLine;<br>
if (ItemKey.Column > 0)<br>
OS << std::string(ItemKey.Column - 1, ' ') << "^\n";<br>
OS << "error: Macro instance '" << *MacroExpTracker.MacroUnexpanded<br>
<< "' has different values in this header, depending on how it was "<br>
"included.\n";<br>
+ // Walk all the instances.<br>
for (std::vector<MacroExpansionInstance>::iterator<br>
IMT = MacroExpTracker.MacroExpansionInstances.begin(),<br>
EMT = MacroExpTracker.MacroExpansionInstances.end();<br>
@@ -1013,6 +1052,7 @@ public:<br>
OS << " '" << *MacroExpTracker.MacroUnexpanded << "' expanded to: '"<br>
<< *MacroInfo.MacroExpanded<br>
<< "' with respect to these inclusion paths:\n";<br>
+ // Walk all the inclusion path hierarchies.<br>
for (std::vector<InclusionPathHandle>::iterator<br>
IIP = MacroInfo.InclusionPathHandles.begin(),<br>
EIP = MacroInfo.InclusionPathHandles.end();<br>
@@ -1046,6 +1086,7 @@ public:<br>
// Returns true if any mismatches.<br>
bool reportInconsistentConditionals(llvm::raw_ostream &OS) {<br>
bool ReturnValue = false;<br>
+ // Walk all the conditional trackers in the map.<br>
for (ConditionalExpansionMapIter I = ConditionalExpansions.begin(),<br>
E = ConditionalExpansions.end();<br>
I != E; ++I) {<br>
@@ -1053,7 +1094,9 @@ public:<br>
ConditionalTracker &CondTracker = I->second;<br>
if (!CondTracker.hasMismatch())<br>
continue;<br>
+ // Tell caller we found one or more errors.<br>
ReturnValue = true;<br>
+ // Start the error message.<br>
OS << *HeaderPaths[ItemKey.File] << ":" << ItemKey.Line << ":"<br>
<< ItemKey.Column << "\n";<br>
OS << "#" << getDirectiveSpelling(CondTracker.DirectiveKind) << " "<br>
@@ -1063,6 +1106,7 @@ public:<br>
<< *CondTracker.ConditionUnexpanded<br>
<< "' has different values in this header, depending on how it was "<br>
"included.\n";<br>
+ // Walk all the instances.<br>
for (std::vector<ConditionalExpansionInstance>::iterator<br>
IMT = CondTracker.ConditionalExpansionInstances.begin(),<br>
EMT = CondTracker.ConditionalExpansionInstances.end();<br>
@@ -1071,6 +1115,7 @@ public:<br>
OS << " '" << *CondTracker.ConditionUnexpanded << "' expanded to: '"<br>
<< (MacroInfo.ConditionValue ? "true" : "false")<br>
<< "' with respect to these inclusion paths:\n";<br>
+ // Walk all the inclusion path hierarchies.<br>
for (std::vector<InclusionPathHandle>::iterator<br>
IIP = MacroInfo.InclusionPathHandles.begin(),<br>
EIP = MacroInfo.InclusionPathHandles.end();<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>