Merge pull request #1700 from KhronosGroup/fix-pp

PP: Remove sub-tokens in macro recording and record spaces correctly.

glslang/MachineIndependent/preprocessor/Pp.cpp

@@ -166,29 +166,43 @@ int TPpContext::CPPdefine(TPpToken* ppToken)
     if (existing != nullptr) {
         if (! existing->undef) {
             // Already defined -- need to make sure they are identical:
-            // "Two replacement lists are identical if and only if the preprocessing tokens in both have the same number,
+            // "Two replacement lists are identical if and only if the
-            // ordering, spelling, and white-space separation, where all white-space separations are considered identical."
+            // preprocessing tokens in both have the same number,
-            if (existing->functionLike != mac.functionLike)
+            // ordering, spelling, and white-space separation, where all
-                parseContext.ppError(defineLoc, "Macro redefined; function-like versus object-like:", "#define", atomStrings.getString(defAtom));
+            // white-space separations are considered identical."
-            else if (existing->args.size() != mac.args.size())
+            if (existing->functionLike != mac.functionLike) {
-                parseContext.ppError(defineLoc, "Macro redefined; different number of arguments:", "#define", atomStrings.getString(defAtom));
+                parseContext.ppError(defineLoc, "Macro redefined; function-like versus object-like:", "#define",
-            else {
+                    atomStrings.getString(defAtom));
-                if (existing->args != mac.args)
+            } else if (existing->args.size() != mac.args.size()) {
-                    parseContext.ppError(defineLoc, "Macro redefined; different argument names:", "#define", atomStrings.getString(defAtom));
+                parseContext.ppError(defineLoc, "Macro redefined; different number of arguments:", "#define",
+                    atomStrings.getString(defAtom));
+            } else {
+                if (existing->args != mac.args) {
+                    parseContext.ppError(defineLoc, "Macro redefined; different argument names:", "#define",
+                       atomStrings.getString(defAtom));
+                }
+                // set up to compare the two
                 existing->body.reset();
                 mac.body.reset();
                 int newToken;
+                bool firstToken = true;
                 do {
                     int oldToken;
                     TPpToken oldPpToken;
                     TPpToken newPpToken;
                     oldToken = existing->body.getToken(parseContext, &oldPpToken);
                     newToken = mac.body.getToken(parseContext, &newPpToken);
+                    // for the first token, preceding spaces don't matter
+                    if (firstToken) {
+                        newPpToken.space = oldPpToken.space;
+                        firstToken = false;
+                    }
                     if (oldToken != newToken || oldPpToken != newPpToken) {
-                        parseContext.ppError(defineLoc, "Macro redefined; different substitutions:", "#define", atomStrings.getString(defAtom));
+                        parseContext.ppError(defineLoc, "Macro redefined; different substitutions:", "#define",
+                            atomStrings.getString(defAtom));
                         break;
                     }
-                } while (newToken > 0);
+                } while (newToken != EndOfInput);
             }
         }
         *existing = mac;

glslang/MachineIndependent/preprocessor/PpContext.h

@@ -243,24 +243,48 @@ public:
     // From PpTokens.cpp
     //
 
+    // Capture the needed parts of a token stream for macro recording/playback.
     class TokenStream {
     public:
-        TokenStream() : current(0) { }
+        // Manage a stream of these 'Token', which capture the relevant parts
+        // of a TPpToken, plus its atom.
+        class Token {
+        public:
+            Token(int atom, const TPpToken& ppToken) : 
+                atom(atom),
+                space(ppToken.space),
+                i64val(ppToken.i64val),
+                name(ppToken.name) { }
+            int get(TPpToken& ppToken)
+            {
+                ppToken.clear();
+                ppToken.space = space;
+                ppToken.i64val = i64val;
+                snprintf(ppToken.name, sizeof(ppToken.name), "%s", name.c_str());
+                return atom;
+            }
+            bool isAtom(int a) { return atom == a; }
+        protected:
+            Token() {}
+            int atom;
+            bool space;        // did a space precede the token?
+            long long i64val;
+            TString name;
+        };
+
+        TokenStream() : currentPos(0) { }
 
         void putToken(int token, TPpToken* ppToken);
+        bool peekToken(int atom) { return !atEnd() && stream[currentPos].isAtom(atom); }
         int getToken(TParseContextBase&, TPpToken*);
-        bool atEnd() { return current >= data.size(); }
+        bool atEnd() { return currentPos >= stream.size(); }
         bool peekTokenizedPasting(bool lastTokenPastes);
         bool peekUntokenizedPasting();
-        void reset() { current = 0; }
+        void reset() { currentPos = 0; }
 
     protected:
-        void putSubtoken(char);
+        TVector<Token> stream;
-        int getSubtoken();
+        size_t currentPos;
-        void ungetSubtoken();
-
-        TVector<unsigned char> data;
-        size_t current;
     };
 
     //

glslang/MachineIndependent/preprocessor/PpTokens.cpp

@@ -99,151 +99,33 @@ NVIDIA HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 namespace glslang {
 
-
+// Add a token (including backing string) to the end of a macro
-namespace {
+// token stream, for later playback.
-
-    // When recording (and playing back) should the backing name string
-    // be saved (restored)?
-    bool SaveName(int atom)
-    {
-        switch (atom) {
-        case PpAtomIdentifier:
-        case PpAtomConstString:
-        case PpAtomConstInt:
-        case PpAtomConstUint:
-        case PpAtomConstInt64:
-        case PpAtomConstUint64:
-    #ifdef AMD_EXTENSIONS
-        case PpAtomConstInt16:
-        case PpAtomConstUint16:
-    #endif
-        case PpAtomConstFloat:
-        case PpAtomConstDouble:
-        case PpAtomConstFloat16:
-            return true;
-        default:
-            return false;
-        }
-    }
-
-    // When recording (and playing back) should the numeric value
-    // be saved (restored)?
-    bool SaveValue(int atom)
-    {
-        switch (atom) {
-        case PpAtomConstInt:
-        case PpAtomConstUint:
-        case PpAtomConstInt64:
-        case PpAtomConstUint64:
-    #ifdef AMD_EXTENSIONS
-        case PpAtomConstInt16:
-        case PpAtomConstUint16:
-    #endif
-        case PpAtomConstFloat:
-        case PpAtomConstDouble:
-        case PpAtomConstFloat16:
-            return true;
-        default:
-            return false;
-        }
-    }
-}
-
-// push onto back of stream
-void TPpContext::TokenStream::putSubtoken(char subtoken)
-{
-    data.push_back(static_cast<unsigned char>(subtoken));
-}
-
-// get the next token in stream
-int TPpContext::TokenStream::getSubtoken()
-{
-    if (current < data.size())
-        return data[current++];
-    else
-        return EndOfInput;
-}
-
-// back up one position in the stream
-void TPpContext::TokenStream::ungetSubtoken()
-{
-    if (current > 0)
-        --current;
-}
-
-// Add a complete token (including backing string) to the end of a list
-// for later playback.
 void TPpContext::TokenStream::putToken(int atom, TPpToken* ppToken)
 {
-    // save the atom
+    TokenStream::Token streamToken(atom, *ppToken);
-    assert((atom & ~0xff) == 0);
+    stream.push_back(streamToken);
-    putSubtoken(static_cast<char>(atom));
-
-    // save the backing name string
-    if (SaveName(atom)) {
-        const char* s = ppToken->name;
-        while (*s)
-            putSubtoken(*s++);
-        putSubtoken(0);
 }
 
-    // save the numeric value
+// Read the next token from a macro token stream.
-    if (SaveValue(atom)) {
-        const char* n = reinterpret_cast<const char*>(&ppToken->i64val);
-        for (size_t i = 0; i < sizeof(ppToken->i64val); ++i)
-            putSubtoken(*n++);
-    }
-}
-
-// Read the next token from a token stream.
-// (Not the source stream, but a stream used to hold a tokenized macro).
 int TPpContext::TokenStream::getToken(TParseContextBase& parseContext, TPpToken *ppToken)
 {
-    // get the atom
+    if (atEnd())
-    int atom = getSubtoken();
+        return EndOfInput;
-    if (atom == EndOfInput)
-        return atom;
 
-    // init the token
+    int atom = stream[currentPos++].get(*ppToken);
-    ppToken->clear();
     ppToken->loc = parseContext.getCurrentLoc();
 
-    // get the backing name string
-    if (SaveName(atom)) {
-        int ch = getSubtoken();
-        int len = 0;
-        while (ch != 0 && ch != EndOfInput) {
-            if (len < MaxTokenLength) {
-                ppToken->name[len] = (char)ch;
-                len++;
-                ch = getSubtoken();
-            } else {
-                parseContext.error(ppToken->loc, "token too long", "", "");
-                break;
-            }
-        }
-        ppToken->name[len] = 0;
-    }
-
     // Check for ##, unless the current # is the last character
     if (atom == '#') {
-        if (current < data.size()) {
+        if (peekToken('#')) {
-            if (getSubtoken() == '#') {
             parseContext.requireProfile(ppToken->loc, ~EEsProfile, "token pasting (##)");
             parseContext.profileRequires(ppToken->loc, ~EEsProfile, 130, 0, "token pasting (##)");
+            currentPos++;
             atom = PpAtomPaste;
-            } else
-                ungetSubtoken();
         }
     }
 
-    // get the numeric value
-    if (SaveValue(atom)) {
-        char* n = reinterpret_cast<char*>(&ppToken->i64val);
-        for (size_t i = 0; i < sizeof(ppToken->i64val); ++i)
-            *n++ = (char)getSubtoken();
-    }
-
     return atom;
 }
 
@@ -256,15 +138,14 @@ bool TPpContext::TokenStream::peekTokenizedPasting(bool lastTokenPastes)
 {
     // 1. preceding ##?
 
-    size_t savePos = current;
+    size_t savePos = currentPos;
-    int subtoken;
     // skip white space
-    do {
+    while (peekToken(' '))
-        subtoken = getSubtoken();
+        ++currentPos;
-    } while (subtoken == ' ');
+    if (peekToken(PpAtomPaste)) {
-    current = savePos;
+        currentPos = savePos;
-    if (subtoken == PpAtomPaste)
         return true;
+    }
 
     // 2. last token and we've been told after this there will be a ##
 
@@ -273,18 +154,18 @@ bool TPpContext::TokenStream::peekTokenizedPasting(bool lastTokenPastes)
     // Getting here means the last token will be pasted, after this
 
     // Are we at the last non-whitespace token?
-    savePos = current;
+    savePos = currentPos;
     bool moreTokens = false;
     do {
-        subtoken = getSubtoken();
+        if (atEnd())
-        if (subtoken == EndOfInput)
             break;
-        if (subtoken != ' ') {
+        if (!peekToken(' ')) {
             moreTokens = true;
             break;
         }
+        ++currentPos;
     } while (true);
-    current = savePos;
+    currentPos = savePos;
 
     return !moreTokens;
 }
@@ -293,23 +174,21 @@ bool TPpContext::TokenStream::peekTokenizedPasting(bool lastTokenPastes)
 bool TPpContext::TokenStream::peekUntokenizedPasting()
 {
     // don't return early, have to restore this
-    size_t savePos = current;
+    size_t savePos = currentPos;
 
     // skip white-space
-    int subtoken;
+    while (peekToken(' '))
-    do {
+        ++currentPos;
-        subtoken = getSubtoken();
-    } while (subtoken == ' ');
 
     // check for ##
     bool pasting = false;
-    if (subtoken == '#') {
+    if (peekToken('#')) {
-        subtoken = getSubtoken();
+        ++currentPos;
-        if (subtoken == '#')
+        if (peekToken('#'))
             pasting = true;
     }
 
-    current = savePos;
+    currentPos = savePos;
 
     return pasting;
 }