Skip to content

Conversation

treeform
Copy link
Contributor

@treeform treeform commented Apr 3, 2018

  • Created new nkQuotedIdent node type.
  • Fixed warning related to toUpperAscii and toLowerAscii.
  • Moved stream related methods under when not js block.
  • Fix order by limit by parsing.

proc repr*(node: SqlNode): string =
result = $node.kind
if node.kind in LiteralNodes:
result &= " \"" & node.strVal & "\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not how to quote things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how this repr works anyways. I'll just remove the whole function for now.

for i, son in node.sons:
if i > 0:
result &= ", "
result &= repr(son)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is expensive, change repr to take result: var string parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right its not ideal. Do other reprs take result: var string?

Copy link
Member

@Araq Araq Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, usually the $ operator is overloaded and it uses a helper proc that uses the var string in the recursions. In fact, overloading of repr is simply not done in the stdlib (except for macros.nim and it's a mistake there too).

@treeform
Copy link
Contributor Author

treeform commented Apr 4, 2018

I have another problem related to this code I don't know how to fix. I want to use this code in JavaScript (client side SQL validation) but it uses lexbase which uses newStringStream which does pointer stuff and memory copy and moves so it can't compile. Should I try to make a JS safe lexbase? or JS safe StringStream? I am at a loss. Or try to make this parser not use lexbase and just use plain strings?

@Araq
Copy link
Member

Araq commented Apr 4, 2018

Should I try to make a JS safe lexbase?

Yeah, go for it. I'll help you out on IRC.

@dom96
Copy link
Contributor

dom96 commented Apr 5, 2018

How I solved this in the json module might be relevant: https://github.com/nim-lang/Nim/blob/devel/lib/pure/json.nim#L1245

result = newNode(nkIdent, p.tok.literal)
getTok(p)
of tkIdentifier:
if p.tok.literal.toLowerAscii() in SqlFunctions:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, that is not how the SQL is supposed to work. We don't know the SQL functions there can be hundreds per SQL dialect. Why special case these few? It also makes the parser slower, tkIdentifier is likely on the critical path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the functions list, but I had to pass Quoted Identifiers from tokens to nodes as there needs to be a way to say quote foo as "count" and also have the unquoted count(*).

nkPrefix,
nkInfix,
nkCall,
nkFunction,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nkFunction is a misguided concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@treeform treeform force-pushed the sqlfix2 branch 2 times, most recently from 238500f to 70a7161 Compare April 10, 2018 17:58
@treeform
Copy link
Contributor Author

@dom96 It looks like you just use the native json parser for javascript and then build nim nodes out out it's output?

@Araq Araq merged commit f3db632 into nim-lang:devel Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants