Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong use search #849

Closed
wants to merge 2 commits into from
Closed

Fix wrong use search #849

wants to merge 2 commits into from

Conversation

horasal
Copy link
Contributor

@horasal horasal commented Jan 9, 2015

Problem Description:

UseDef findUse() return wrong path according to the order of soures.

Assume that we have ./ooc-math.use and ./source/sdk/math.use. When searching for math.use, the following code

if(subPath getPath() endsWith?(fileName))

gives ooc-math.use instead of math.use because both of them have suffixes "math.use" but ooc-math is searched first. This will cause circle dependency problem.

How to reproduce:

Compile ooc-kean with the lastest rock.

Fix:

  • Added getName() to extract filename from path.
  • When getName() does not work well, it will return the first match of endWiths.

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

Tried to reproduce by building ooc-kean, but it's failing earlier with a redefinition of abs, which seems weird:

screen shot 2015-01-12 at 16 29 45

fasterthanlime added a commit that referenced this issue Jan 12, 2015
@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

@zhaihj Can you please have a look at da917b2 ? That should fix it.

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jan 12, 2015

Goodness, that's a big text size for a terminal. 4K?

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jan 12, 2015

Btw, just fixed the abs problem, which mysteriously appeared today.

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

@davidhesselbom Just a retina display :) (native resolution: 2880x1800).

About abs: I think it might be related to the stricter checks introduced in 80e0a09

(Which, in retrospect, explains a0c178c). Imho type extensions should be able to redefine functions if they feel like it, although that might be too obscure. Hm.

@vendethiel
Copy link

@vendethiel vendethiel commented Jan 12, 2015

Do you want to open ruby's biggest can of worms :)?

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

@vendethiel d'awwwwww good point let's not talk about open classes or monkey-patching or any of that scripting language nonsense.

(But, btw, type extensions in ooc are a lie — and completely bypass the vtable.)

@vendethiel
Copy link

@vendethiel vendethiel commented Jan 12, 2015

Sorry, it's been a long time since I last looked at some rock, but I just think... It's a bad idea to add that :P.

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

Actually it's been a very long time since I've read some parts of rock. And when I read it now, I just want to burn it all. To the ground. And rewrite it all. But who's got the time for that :(

Cogneco when you're rich, hire me, I'll make ooc better I promise.

@vendethiel
Copy link

@vendethiel vendethiel commented Jan 12, 2015

Would you have, in retrospect, liked better to make ooc a "google" language? (as I think you got an email from google or smth)

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

I did get an e-mail from Google about a job offer — but they send hundreds of those everyday, it was nothing serious / personal. And it wasn't ooc-related as far as I could tell.

I've used ooc professionally at official.fm, and I've written another compiler at my current company, and I'm still today using ooc in another project, with yet another language on top — so, you know, I'm still "in the game" somewhat, just not running around, giving conferences and whatnot.

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jan 12, 2015

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jan 12, 2015

Just, you know, in case you're curious.

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 12, 2015

@davidhesselbom that looks really cool :)

@vendethiel
Copy link

@vendethiel vendethiel commented Jan 12, 2015

Alright - amazing. Did see that new compiler :).

@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 13, 2015

@davidhesselbom well, you know, if you've got some extra funding, I wouldn't mind spending some time doing paid work to improve ooc.

I might have to look into... alternative solutions of a financial kind, for the near future. Oh well.

@davidhesselbom
Copy link
Contributor

@davidhesselbom davidhesselbom commented Jan 14, 2015

@fasterthanlime How much to fix #829? ;)

(actually, that one's been fixed, I just haven't had time to make a pull request yet. I haven't tried zhaihj's fix either, it might work even better than ours...)

I'll see if there's some extra money lying around the office, the idea has been brought up before.

@horasal
Copy link
Contributor Author

@horasal horasal commented Jan 14, 2015

@fasterthanlime
that commit works perferctly. I will close this pr...

@horasal horasal closed this Jan 14, 2015
@fasterthanlime
Copy link
Collaborator

@fasterthanlime fasterthanlime commented Jan 14, 2015

Thanks for confirming @zhaihj !

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.

None yet

4 participants