[clang-tools-extra] r187370 - Fixed comment issues (non-ASCII chars, typos) per review, expanded some comments.

[Richard Smith]

On Mon, Jul 29, 2013 at 12:07 PM, John Thompson <
John.Thompson.JTSoftware at gmail.com> wrote:

> Author: jtsoftware
> Date: Mon Jul 29 14:07:00 2013
> New Revision: 187370
>
> URL: http://llvm.org/viewvc/llvm-project?rev=187370&view=rev
> Log:
> Fixed comment issues (non-ASCII chars, typos) per review, expanded some
> comments.
>
> Modified:
>     clang-tools-extra/trunk/modularize/Modularize.cpp
>     clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp
>
> Modified: clang-tools-extra/trunk/modularize/Modularize.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/Modularize.cpp?rev=187370&r1=187369&r2=187370&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/modularize/Modularize.cpp (original)
> +++ clang-tools-extra/trunk/modularize/Modularize.cpp Mon Jul 29 14:07:00
> 2013
> @@ -35,17 +35,43 @@
>  // Modularize will do normal parsing, reporting normal errors and
> warnings,
>  // but will also report special error messages like the following:
>  //
> -// error: '(symbol)' defined at multiple locations:
> -//     (file):(row):(column)
> -//     (file):(row):(column)
> +//   error: '(symbol)' defined at multiple locations:
> +//       (file):(row):(column)
> +//       (file):(row):(column)
>  //
> -// error: header '(file)' has different contents dependening on how it was
> -//   included
> +//   error: header '(file)' has different contents dependening on how it
> was
> +//     included
>

Dependening?


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