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 <dave.birds...@esgyn.com> 
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 <selva.govindara...@esgyn.com> 
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 <dave.birds...@esgyn.com> 
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

Reply via email to