Skip to content

Conversation

bbodi
Copy link

@bbodi bbodi commented Apr 23, 2014

No description provided.

@dom96
Copy link
Contributor

dom96 commented Apr 23, 2014

Nice! I think the URL concatenation should concatenate the path and query instead of just picking one though.

let u = parseUrl("http://localhost/")
let u2 = parseUrl("/foo?test=4&test2=5")
let u3 = parseUrl("/bar?q=123")
assert (u / u2 / u3) == parseUrl("http://localhost/foo/bar?test=4&test2=5&q=123")

Would be worth checking what URL modules in other languages do though.

@Varriount
Copy link
Contributor

This has my ok. @dom96, do you have any concerns?

@dom96
Copy link
Contributor

dom96 commented Apr 27, 2014

Yeah? Did you read my previous comment?

On Sunday, April 27, 2014, Varriount notifications@github.com wrote:

This has my ok. @dom96 https://github.com/dom96, do you have any
concerns?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1139#issuecomment-41484062
.

Varriount added a commit that referenced this pull request Apr 27, 2014
@Varriount Varriount merged commit 2da7ad8 into nim-lang:devel Apr 27, 2014
@dom96
Copy link
Contributor

dom96 commented Apr 27, 2014

Why did you pull that?

On Sunday, April 27, 2014, Varriount notifications@github.com wrote:

Merged #1139 #1139.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1139
.

@Varriount
Copy link
Contributor

I took your "Yeah" to mean yes. Did I misinterpret?

@dom96
Copy link
Contributor

dom96 commented Apr 27, 2014

Yes. You asked whether i have any concerns.

On Sunday, April 27, 2014, Varriount notifications@github.com wrote:

I took your "Yeah" to mean yes. Did I misinterpret?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1139#issuecomment-41485303
.

@Varriount
Copy link
Contributor

Gah, sorry. I misread.

@dom96
Copy link
Contributor

dom96 commented Apr 27, 2014

It's ok. Now you can implement my suggestion :p

On Sunday, April 27, 2014, Varriount notifications@github.com wrote:

Gah, sorry. I misread.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1139#issuecomment-41485411
.

@Varriount
Copy link
Contributor

Well, from what I can see on python's urlparse module, the current behavior of URL concatenation follows roughly what is done there. Plus, the current structure of a URL object doesn't really allow for concatenation like a string, since the pieces of information are stored in various attributes.

@dom96
Copy link
Contributor

dom96 commented Apr 27, 2014

I think it would be nice to get the behaviour i specified above, please
implement it. Unless you can think of a serious drawback.

On Sunday, April 27, 2014, Varriount notifications@github.com wrote:

Well, from what I can see on python's urlparse modulehttps://docs.python.org/2/library/urlparse.html,
the current behavior of URL concatenation follows roughly what is done
there. Plus, the current structure of a URL object doesn't really allow for
concatenation like a string, since the pieces of information are stored in
various attributes.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1139#issuecomment-41485488
.

Varriount added a commit to Varriount/Nim that referenced this pull request Apr 27, 2014
This reverts commit 2da7ad8, reversing
changes made to 8c6b1b4.
Varriount added a commit to Varriount/Nim that referenced this pull request Apr 27, 2014
This reverts commit 2da7ad8, reversing
changes made to 8c6b1b4.
Varriount added a commit that referenced this pull request Apr 27, 2014
Revert "Merge pull request #1139 from bbodi/devel"
@Varriount
Copy link
Contributor

@bbodi Sorry for my mistakes (that'll teach me to be pull-happy on a late saturday night). Please open another PR so that we can discuss what might need to be changed.

Clyybber pushed a commit to Clyybber/Nim that referenced this pull request Feb 24, 2024
## Summary

In the mid- and backend, there's now a clear distinction being made
between *constants* and *constant data*. A *constant* (i.e., a `const`)
links a name/interface with *constant data*, while *constant data* is
just the raw data.

Storage of constant data is now also decoupled from `TSym.ast`,
preparing for the introduction of a dedicated constant data IR.

## Details

#### General architecture

* same as before, constant data is represented by a `PNode` AST
* every complete `PNode` AST representing constant data is identified
  by the new `DataId`
* all constant data the mid- and backend know about is stored in
  `MirEnv` (in a `DataTable`)
* the association between a `DataId` and the data is *bi-directional*
  (a `DataId` maps to a constant expression and a constant expression
  maps to a `DataId`)
* each user-defined constant (`ConstId`) is associated with a `DataId`
* when discovering user-defined constants, `backends.process` and
  `backends.discover` register the data with the environment and link
  it with the constant

#### Rationale

Separating constants from their data allows two constants to share the
same `PNode` AST internally (at least in the backend). It also makes it
easy for the mid-end to create new constant data (e.g., by lifting
constant expression from procedure bodies) itself -- no name nor
interface has to be associated with the data in this case.

#### Implementation

* `DataTable` is a customized version of `BiTable`
* the `PNode` ASTs representing the constant expression are compared by
  their structure
* using structural comparison means that `Obj(a: 0)` and `Obj(b: 0)`
  are treated as being different, even though they represent the same
  data. While not critical, a normalization pass is going be needed
  here, eventually
* the `DataTable` also supports the basic rewind mechanism employed by
  `MirEnv`
* as an interim solution, a separate table is used for mapping
  constants to their body (`MirEnv.bodies`)

#### Code generation

* instead of querying `TSym.ast`, `cgen` and `jsgen` lookup the body
  for constant through `MirEnv`
* constant data in the VM maps directly to the `DataTable` now (that
  is, each item in `DataTable` maps to a complex VM constant). `vmgen`,
  `vmjit`, and `vmbackend` are adjusted accordingly
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