RE: Reserved/Nonreserved word cruft clean-up
Hi, Just wanted to follow up this discussion. I've done some work on this issue. In the e-mails below I noted that the ComResWords.cpp code was dead in Trafodion. Turns out this was a bug; the code should not have been dead (see JIRA TRAFODION-3131). I've done some clean-ups and others are in progress. I explored refactoring the code in https://issues.apache.org/jira/browse/TRAFODION-3131. I decided that this wasn't a good idea. Instead, I added extensive notes to parser/ParKeyWords.cpp so that future developers adding new keywords to Trafodion will consider all the necessary places in the code. I also included some other bug fixes. That change was merged into Trafodion earlier this week. I removed the obsolete MP-isms from parser/ParKeyWords.cpp and common/ComResWords.cpp in https://issues.apache.org/jira/browse/TRAFODION-3135. A pull request with those changes is presently outstanding. If I don't get any negative comments, I'll likely merge it early next week. While researching this latter change, I noticed lots of other obsolete MP-isms scattered around the parser and other parts of the compiler. I noted these in https://issues.apache.org/jira/browse/TRAFODION-3139, which I'll address in my spare time over the coming weeks unless someone beats me to it. I tried doing part of that work with JIRA TRAFODION-3135, but ran into complications that I didn't want to deal with just then. Better to take a more leisurely approach. Dave -Original Message- From: Dave Birdsall Sent: Tuesday, July 3, 2018 12:55 PM To: dev@trafodion.apache.org Subject: RE: Reserved/Nonreserved word cruft clean-up Hi, It may be that folks are making changes to it, but the code itself is quite dead. I performed the following experiment: I commented out all the code in common/ComResWords.h and common/ComResWords.cpp, and rebuilt Trafodion. It builds cleanly, without compile or link errors. Just for grins, I ran fullstack2 regressions. They pass. Dave -Original Message- From: Selva Govindarajan Sent: Tuesday, July 3, 2018 12:07 PM To: dev@trafodion.apache.org Subject: RE: Reserved/Nonreserved word cruft clean-up It is good to make this change to avoid confusion and to make code consistent and uniform in all places. However, just want to point out that ComResWords.cpp is still in active use. There has been changes in this file as recent as Apr via https://github.com/apache/trafodion/pull/1485 Thanks Selva -Original Message- From: Dave Birdsall Sent: Tuesday, July 3, 2018 11:58 AM To: dev@trafodion.apache.org Subject: Reserved/Nonreserved word cruft clean-up Hi, Trafodion today has three places where it defines whether a word is a reserved word or a non-reserved word. In parser/ParKeyWords.cpp, there is a table of identifiers. Each identifier has a token code associated with it. If the token code is not IDENTIFIER, it may be a reserved or non-reserved word. There are flags, RESWORD_, NONRESWORD_ and NONRESTOKEN_ associated with each identifier in the table, however logic never looks at these flags; they are for documentation purposes only. (Exception: There is legacy code that looks at them if we are in an "MP" context. That refers to one of Trafodion's predecessor products. That code is never entered in Trafodion.) The real work happens in parser/sqlparser.y. If a word's token has a token code other than IDENTIFIER, then there are a set of productions that determine whether it is reserved or not. If it is in the nonreserved_word production, then it is a non-reserved word. So apart from those contexts where the word has a special meaning, it can be used as an ordinary identifier. Similarly, nonreserved_func_word lists tokens that are acceptable as function names. Each of these productions allows a word to be an ordinary identifier in particular contexts. If a word's token is not in any of these productions, then it is truly a reserved word. There is another module, common/ComResWords.cpp, that seems to duplicate the table in parser/ParKeyWords.cpp, but it is quite out-of-date. It's also never called. It looks like in the past it was used by the ToInternalIdentifier function in common/NAString.cpp to return an error if one tried to use a reserved word as an external-format identifier. But that logic has been changed; ToInternalIdentifier now calls a stub function that always returns FALSE when checking for reserved words. I'd like to simplify this logic, as it is causing problems for folks who are adding new non-reserved words to Trafodion. I'd like to delete common/ComResWords.cpp entirely. It's unused, obsolete and redundant. If we decide we need ToInternalIdentifier to check for reserved words, we can have it use the table in parser/ParKeyWords.cpp instead. I'd also like to remove the obsolete "MP context" code in parser/ParKeyWords.cp
RE: Reserved/Nonreserved word cruft clean-up
Hi, It may be that folks are making changes to it, but the code itself is quite dead. I performed the following experiment: I commented out all the code in common/ComResWords.h and common/ComResWords.cpp, and rebuilt Trafodion. It builds cleanly, without compile or link errors. Just for grins, I ran fullstack2 regressions. They pass. Dave -Original Message- From: Selva Govindarajan Sent: Tuesday, July 3, 2018 12:07 PM To: dev@trafodion.apache.org Subject: RE: Reserved/Nonreserved word cruft clean-up It is good to make this change to avoid confusion and to make code consistent and uniform in all places. However, just want to point out that ComResWords.cpp is still in active use. There has been changes in this file as recent as Apr via https://github.com/apache/trafodion/pull/1485 Thanks Selva -Original Message- From: Dave Birdsall Sent: Tuesday, July 3, 2018 11:58 AM To: dev@trafodion.apache.org Subject: Reserved/Nonreserved word cruft clean-up Hi, Trafodion today has three places where it defines whether a word is a reserved word or a non-reserved word. In parser/ParKeyWords.cpp, there is a table of identifiers. Each identifier has a token code associated with it. If the token code is not IDENTIFIER, it may be a reserved or non-reserved word. There are flags, RESWORD_, NONRESWORD_ and NONRESTOKEN_ associated with each identifier in the table, however logic never looks at these flags; they are for documentation purposes only. (Exception: There is legacy code that looks at them if we are in an "MP" context. That refers to one of Trafodion's predecessor products. That code is never entered in Trafodion.) The real work happens in parser/sqlparser.y. If a word's token has a token code other than IDENTIFIER, then there are a set of productions that determine whether it is reserved or not. If it is in the nonreserved_word production, then it is a non-reserved word. So apart from those contexts where the word has a special meaning, it can be used as an ordinary identifier. Similarly, nonreserved_func_word lists tokens that are acceptable as function names. Each of these productions allows a word to be an ordinary identifier in particular contexts. If a word's token is not in any of these productions, then it is truly a reserved word. There is another module, common/ComResWords.cpp, that seems to duplicate the table in parser/ParKeyWords.cpp, but it is quite out-of-date. It's also never called. It looks like in the past it was used by the ToInternalIdentifier function in common/NAString.cpp to return an error if one tried to use a reserved word as an external-format identifier. But that logic has been changed; ToInternalIdentifier now calls a stub function that always returns FALSE when checking for reserved words. I'd like to simplify this logic, as it is causing problems for folks who are adding new non-reserved words to Trafodion. I'd like to delete common/ComResWords.cpp entirely. It's unused, obsolete and redundant. If we decide we need ToInternalIdentifier to check for reserved words, we can have it use the table in parser/ParKeyWords.cpp instead. I'd also like to remove the obsolete "MP context" code in parser/ParKeyWords.cpp (and possibly other places in the parser). Does anyone object to my doing this? If I hear no objections by Monday, July 8, I'll file a JIRA and commence work. Thanks, Dave
RE: Reserved/Nonreserved word cruft clean-up
It is good to make this change to avoid confusion and to make code consistent and uniform in all places. However, just want to point out that ComResWords.cpp is still in active use. There has been changes in this file as recent as Apr via https://github.com/apache/trafodion/pull/1485 Thanks Selva -Original Message- From: Dave Birdsall Sent: Tuesday, July 3, 2018 11:58 AM To: dev@trafodion.apache.org Subject: Reserved/Nonreserved word cruft clean-up Hi, Trafodion today has three places where it defines whether a word is a reserved word or a non-reserved word. In parser/ParKeyWords.cpp, there is a table of identifiers. Each identifier has a token code associated with it. If the token code is not IDENTIFIER, it may be a reserved or non-reserved word. There are flags, RESWORD_, NONRESWORD_ and NONRESTOKEN_ associated with each identifier in the table, however logic never looks at these flags; they are for documentation purposes only. (Exception: There is legacy code that looks at them if we are in an "MP" context. That refers to one of Trafodion's predecessor products. That code is never entered in Trafodion.) The real work happens in parser/sqlparser.y. If a word's token has a token code other than IDENTIFIER, then there are a set of productions that determine whether it is reserved or not. If it is in the nonreserved_word production, then it is a non-reserved word. So apart from those contexts where the word has a special meaning, it can be used as an ordinary identifier. Similarly, nonreserved_func_word lists tokens that are acceptable as function names. Each of these productions allows a word to be an ordinary identifier in particular contexts. If a word's token is not in any of these productions, then it is truly a reserved word. There is another module, common/ComResWords.cpp, that seems to duplicate the table in parser/ParKeyWords.cpp, but it is quite out-of-date. It's also never called. It looks like in the past it was used by the ToInternalIdentifier function in common/NAString.cpp to return an error if one tried to use a reserved word as an external-format identifier. But that logic has been changed; ToInternalIdentifier now calls a stub function that always returns FALSE when checking for reserved words. I'd like to simplify this logic, as it is causing problems for folks who are adding new non-reserved words to Trafodion. I'd like to delete common/ComResWords.cpp entirely. It's unused, obsolete and redundant. If we decide we need ToInternalIdentifier to check for reserved words, we can have it use the table in parser/ParKeyWords.cpp instead. I'd also like to remove the obsolete "MP context" code in parser/ParKeyWords.cpp (and possibly other places in the parser). Does anyone object to my doing this? If I hear no objections by Monday, July 8, I'll file a JIRA and commence work. Thanks, Dave
