Feature/source loc review (#672)

* Fixes/improvements based around review comments.

* SourceUnit -> SourceView

* * Removed the HumaneSourceLoc as it's POD-like ness seemed to make that unnecessary
* Made exposed member variables in SourceManager protected - so make clear where/how can be accesed
* Improved description about SourceLoc and associated structures

* Changed SourceLocType to 'Actual' and 'Nominal'.

* Improved a comment.

1 files changed, 79 insertions, 60 deletions

diff --git a/source/slang/source-loc.h b/source/slang/source-loc.h
index 45d99ce3e..48b7791c7 100644
--- a/source/slang/source-loc.h
+++ b/source/slang/source-loc.h
@@ -13,12 +13,29 @@ namespace Slang {
 
 /** Overview: 
 
-SourceFile - Is the immutable contents of a file (or perhaps some generated source - say from doing a macro substitution)
-SourceUnit - Tracks a single parse of a SourceFile. Each SourceUnit defines a range of source locations used. If a SourceFile is parsed twice, two 
-SourceUnits are used, with unique SourceRanges - such that it is possible to tell which specific parse a SourceLoc is from. Not only that but the SourceUnit 
-contains the modifications of the interpretations of source (say by #line) directives. It is necessary to have these different 'views' on the 
-source because if a file is included twice, it may be used in different ways and have different #line directives.
-
+There needs to be a mechanism where we can easily and quickly track a specific locations in any source file used during a compilation. 
+This is important because that original location is meaningful to the user as it relates to their original source. Thus SourceLoc are 
+used so we can display meaningful and accurate errors/warnings as well as being able to always map generated code locations back to their origins.
+
+A 'SourceLoc' along with associated structures (SourceView, SourceFile, SourceMangager) this can pinpoint the location down to the byte across the 
+compilation. This could be achieved by storing for every token and instruction the file, line and column number came from. The SourceLoc is used in 
+lots of places - every AST node, every Token from the lexer, every IRInst - so we really want to make it small. So for this reason we actually 
+encode SourceLoc as a single integer and then use the associated structures when needed to determine what the location actually refers to - 
+the source file, line and column number, or in effect the byte in the original file.  
+
+Unfortunately there is extra complications. When a source is parsed it's interpretation (in terms of how a piece of source maps to an 'original' file etc)
+can be overridden - for example by using #line directives. Moreover a single source file can be parsed multiple times. When it's parsed multiple times the 
+interpretation of the mapping (#line directives for example) can change. This is the purpose of the SourceView - it holds the interpretation of a source file 
+for a specific Lex/Parse. 
+
+Another complication is that not all 'source' comes from SourceFiles, a macro expansion, may generate new 'source' we need to handle this, but also be able 
+to have a SourceLoc map to the expansion unambiguously. This is handled by creating a SourceFile and SourceView that holds only the macro generated 
+specific information.  
+
+SourceFile - Is the immutable text contents of a file (or perhaps some generated source - say from doing a macro substitution)
+SourceView - Tracks a single parse of a SourceFile. Each SourceView defines a range of source locations used. If a SourceFile is parsed twice, two 
+SourceViews are created, with unique SourceRanges. This is so that it is possible to tell which specific parse a SourceLoc is from - and so know the right
+interpretation for that lex/parse. 
 */
 
 class SourceLoc
@@ -104,14 +121,9 @@ public:
         /// Calculate the offset for a line
     int calcColumnIndex(int line, int offset);
 
-    // The logical file path to report for locations inside this span.
-    String path;
-
-    /// A blob that owns the storage for the file contents
-    ComPtr<ISlangBlob> contentBlob;
-
-    /// The actual contents of the file.
-    UnownedStringSlice content;
+    String path;                        ///< The logical file path to report for locations inside this span.
+    ComPtr<ISlangBlob> contentBlob;     ///< A blob that owns the storage for the file contents
+    UnownedStringSlice content;         ///< The actual contents of the file.
 
     protected:
     // In order to speed up lookup of line number information,
@@ -120,12 +132,10 @@ public:
     List<uint32_t> m_lineBreakOffsets;
 };
 
-struct SourceManager;
-
 enum class SourceLocType
 {
-    Normal,                 ///< Takes into account #line directives
-    Original,               ///< Ignores #line directives - humane location as seen in the actual file
+    Nominal,                ///< The normal interpretation which takes into account #line directives 
+    Actual,                 ///< Ignores #line directives - and is the location as seen in the actual file
 };
 
 // A source location in a format a human might like to see
@@ -134,17 +144,16 @@ struct HumaneSourceLoc
     String  path;
     Int     line = 0;
     Int     column = 0;
-
-    String const& getPath() const { return path; }
-    Int getLine() const { return line; }
-    Int getColumn() const { return column; }
 };
 
-/* A SourceUnit maps to a single span of SourceLoc range and is equivalent to a single include or use of a source file. 
+// Pre-declare
+struct SourceManager;
+
+/* A SourceView maps to a single span of SourceLoc range and is equivalent to a single include or more precisely use of a source file. 
 It is distinct from a SourceFile - because a SourceFile may be included multiple times, with different interpretations (depending 
-on #defines for example).s
+on #defines for example).
 */ 
-class SourceUnit: public RefObject
+class SourceView: public RefObject
 {
     public:
 
@@ -152,7 +161,7 @@ class SourceUnit: public RefObject
     // all map to the same logical file.
     struct Entry
     {
-            /// True if this resets the line numbering. It is distinct from a m_lineAdjust from 0, because it also means the path returns to the default.
+            /// True if this resets the line numbering. It is distinct from a m_lineAdjust being 0, because it also means the path returns to the default.
         bool isDefault() const { return m_pathHandle == StringSlicePool::Handle(0); }
 
         SourceLoc m_startLoc;                       ///< Where does this entry begin?
@@ -165,34 +174,36 @@ class SourceUnit: public RefObject
         /// into the underlying sourceFile.
     int findEntryIndex(SourceLoc sourceLoc) const;
 
-        /// Add a line directive for this unit. The directiveLoc must of course be in this SourceUnit
-        /// The path handle, must have been constructed on the SourceManager associated with the unit
+        /// Add a line directive for this view. The directiveLoc must of course be in this SourceView
+        /// The path handle, must have been constructed on the SourceManager associated with the view
         /// NOTE! Directives are assumed to be added IN ORDER during parsing such that every directiveLoc > previous 
     void addLineDirective(SourceLoc directiveLoc, StringSlicePool::Handle pathHandle, int line);
-
     void addLineDirective(SourceLoc directiveLoc, const String& path, int line);
 
         /// Removes any corrections on line numbers and reverts to the source files path
     void addDefaultLineDirective(SourceLoc directiveLoc);
 
-        /// Get the range that this unit applies to
+        /// Get the range that this view applies to
     const SourceRange& getRange() const { return m_range; }
         /// Get the entries
     const List<Entry>& getEntries() const { return m_entries; }
-        /// Get the source file holds the contents this 'unit' 
+        /// Get the source file holds the contents this view 
     SourceFile* getSourceFile() const { return m_sourceFile; }
         /// Get the source manager
     SourceManager* getSourceManager() const { return m_sourceManager; }
 
+        /// Get the associated 'content' (the source text)
+    const UnownedStringSlice& getContent() const { return m_sourceFile->content; }
+
         /// Get the humane location 
         /// Type determines if the location wanted is the original, or the 'normal' (which modifys behavior based on #line directives)
-    HumaneSourceLoc getHumaneLoc(SourceLoc loc, SourceLocType type = SourceLocType::Normal);
+    HumaneSourceLoc getHumaneLoc(SourceLoc loc, SourceLocType type = SourceLocType::Nominal);
 
         /// Get the path associated with a location
-    String getPath(SourceLoc loc, SourceLocType type = SourceLocType::Normal);
+    String getPath(SourceLoc loc, SourceLocType type = SourceLocType::Nominal);
 
         /// Ctor
-    SourceUnit(SourceManager* sourceManager, SourceFile* sourceFile, SourceRange range):
+    SourceView(SourceManager* sourceManager, SourceFile* sourceFile, SourceRange range):
         m_sourceManager(sourceManager),
         m_range(range),
         m_sourceFile(sourceFile)
@@ -202,39 +213,41 @@ class SourceUnit: public RefObject
     protected:
     
     SourceManager* m_sourceManager;     /// Get the manager this belongs to 
-    SourceRange m_range;                ///< The range that this SourceUnit applies to
+    SourceRange m_range;                ///< The range that this SourceView applies to
     RefPtr<SourceFile> m_sourceFile;    ///< The source file can hold the line breaks
     List<Entry> m_entries;              ///< An array entries describing how we should interpret a range, starting from the start location. 
 };
 
 struct SourceManager
 {
-    // Initialize a source manager, with an optional parent
-    void initialize(
-        SourceManager*  parent);
+        // Initialize a source manager, with an optional parent
+    void initialize(SourceManager*  parent);
 
+        /// Allocate a range of SourceLoc locations, these can be used to identify a specific location in the source
     SourceRange allocateSourceRange(UInt size);
 
-    SourceFile* newSourceFile(
-        String const&   path,
-        ISlangBlob*     content);
-
-    SourceFile* newSourceFile(
-        String const&   path,
-        String const&   content);
+        /// Create a SourceFile defined with the specified path, and content held within a blob
+    SourceFile* createSourceFile(String const& path, ISlangBlob* content);
+        /// Create a SourceFile with specified path. Create a Blob that contains the content.
+    SourceFile* createSourceFile(String const& path, String const& content);
 
         /// Get the humane source location
-    HumaneSourceLoc getHumaneLoc(SourceLoc loc, SourceLocType type = SourceLocType::Normal);
+    HumaneSourceLoc getHumaneLoc(SourceLoc loc, SourceLocType type = SourceLocType::Nominal);
 
         /// Get the path associated with a location 
-    String getPath(SourceLoc loc, SourceLocType type = SourceLocType::Normal);
+    String getPath(SourceLoc loc, SourceLocType type = SourceLocType::Nominal);
+
+        /// Create a new source view from a file
+    SourceView* createSourceView(SourceFile* sourceFile);
 
-        /// Allocate a new source unit from a file
-    SourceUnit* newSourceUnit(SourceFile* sourceFile);
+        /// Find a view by a source file location. 
+        /// If not found in this manager will look in the parent SourceManager
+        /// Returns nullptr if not found.
+    SourceView* findSourceViewRecursively(SourceLoc loc) const;
 
-        /// Find a unit by a source file location. If not found in this will look in the parent/
-        /// Returns nullptr if not found
-    SourceUnit* findSourceUnit(SourceLoc loc);
+        /// Find the SourceView associated with this manager for a specified location
+        /// Returns nullptr if not found. 
+    SourceView* findSourceView(SourceLoc loc) const;
 
         /// Searches this manager, and then the parent to see if can find a match for path. 
         /// If not found returns nullptr.    
@@ -246,27 +259,33 @@ struct SourceManager
         /// Get the slice pool
     StringSlicePool& getStringSlicePool() { return m_slicePool; }
 
+        /// Get the source range for just this manager
+        /// Caution - the range will change if allocations are made to this manager.
+    SourceRange getSourceRange() const { return SourceRange(m_startLoc, m_nextLoc); } 
+    
+        /// Get the parent manager to this manager. Returns nullptr if there isn't any.
+    SourceManager* getParent() const { return m_parent; }
+
+    protected:
+
     // The first location available to this source manager
     // (may not be the first location of all, because we might
     // have a parent source manager)
-    SourceLoc startLoc;
+    SourceLoc m_startLoc;
 
     // The "parent" source manager that owns locations ahead of `startLoc`
-    SourceManager* parent = nullptr;
+    SourceManager* m_parent = nullptr;
 
     // The location to be used by the next source file to be loaded
-    SourceLoc nextLoc;
-
-    protected:
+    SourceLoc m_nextLoc;
 
-    // All of the source units. These are held in increasing order of range, so can find by doing a binary chop.
-    List<RefPtr<SourceUnit> > m_sourceUnits;                
+    // All of the SourceViews. These are held in increasing order of range, so can find by doing a binary chop.
+    List<RefPtr<SourceView> > m_sourceViews;                
     StringSlicePool m_slicePool;
 
     Dictionary<String, RefPtr<SourceFile> > m_sourceFiles;
 };
 
-
 } // namespace Slang
 
 #endif