Skip to content

Rustup + The Great Refactoring #36

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

Merged
merged 1 commit into from
Aug 7, 2016
Merged

Rustup + The Great Refactoring #36

merged 1 commit into from
Aug 7, 2016

Conversation

aochagavia
Copy link
Contributor

Main changes:

  • Updated Rust version
  • Split up the code into more modules
  • Removed ast_map.rs, which was a copy of part of
    librustc/front/map/mod.rs. The only difference with the original map was
    removing a TypedArena from the Forest struct

Todo:

Main changes:
* Updated Rust version
* Split up the code into more modules
* Removed ast_map.rs, which was a copy of part of
librustc/front/map/mod.rs. The only difference with the original map was
removing a `TypedArena` from the Forest struct

Todo:
* Fix lifetime walker, since `visit_opt_lifetime_ref` doesn't exist
anymore (see rust-lang/rust#28715)
@aochagavia
Copy link
Contributor Author

UPDATE 2: the crashes seem to be related to the Windows issue described in the readme. I am going to install Ubuntu to check if it works and eventually continue development there.

UPDATE: I am now able to run the tests (switching to the msvc ABI solved it)... However, they don't pass after my changes. I had expected everything to succeed, except lifetime elision and reification. I'll try to fix it, but I still don't know what to do about the lifetime walker.


@nrc I was unable to run the tests because of a strange error (see below). Would you mind to run the tests yourself? I expect the lifetime related refactorings to fail, but the others should be ok (I hope so, after such a huge refactoring).

BTW, do you think this is a rustc bug? Running tests works normally in other projects...

The error:
The error

@nrc
Copy link
Collaborator

nrc commented Dec 7, 2015

@aochagavia did you have any success? Sorry I haven't looked at this - I just got back from vacation. I'll try and take a look this week.

@aochagavia
Copy link
Contributor Author

@nrc The tests compile and run now in Ubuntu, but they fail. The problem is that the compiler doesn't link the correct libraries when we call it from the refactorer. The instructions in the readme haven't helped me so far and I have run out of time, so I think I will wait until Frebruary to try again.

I hope I will be able to make the tool easier to set up, since the current procedure seems very cumbersome and could make adoption difficult.

BTW:

  • I migrated a lot of code to use the HIR, since the Map now contains a hir::Crate instead of a ast::Crate.
  • There is still some code duplication and I want to refactor it further. I think it is important to have a clear and extensible architecture before starting with the real work.

@nrc
Copy link
Collaborator

nrc commented Dec 7, 2015

@GSam - did you ever have this problem running tests? Are there paths you need to set before running?

@aochagavia why do you use the HIR rather than the AST? In general, you should prefer the AST since that is an interface for tools, but the HIR is compiler-internal. If there is something we can fix in Rust, we should do that rather than change to using the HIR here.

@aochagavia
Copy link
Contributor Author

@nrc I moved stuff to the HIR just to get things to compile again. I guess I could migrate it back once I get the refactorer working.

@nrc
Copy link
Collaborator

nrc commented Dec 7, 2015

We should definitely do that and be aware of how much effort it would be (if we need to fix stuff on the compiler side, in particular). One thing that would be nice to do is to use rustfmt for the output, and that requires AST, rather than HIR.

@aochagavia
Copy link
Contributor Author

Would it be desirable to use syntex instead of the AST?

@GSam
Copy link
Owner

GSam commented Dec 7, 2015

The only thing you should need to set is the RUST_FOLDER environment variable which needs to point to the lib folder for Rust (either stage2/lib or stage2/lib/rustlib/[some environment]/lib). I can't really say much more unless there's a specific error message that you see.

@nrc
Copy link
Collaborator

nrc commented Dec 7, 2015

@aochagavia we could. The only benefit of Syntex is that it allows us to work on stable Rust. However, since the tool interacts with other parts of the compiler, I don't think we can be stable anyway. It might be worth looking at later on. In the short term, I don't think it matters too much one way or the other.

@GSam thanks!

@aochagavia
Copy link
Contributor Author

@GSam So all I need is building a stage2 compiler, compiling the tool itself using it and setting the right environment variables? I am going to try it out, thanks!

@GSam
Copy link
Owner

GSam commented Dec 7, 2015

@aochagavia It should probably still work with a regular system install (pointing to some other lib folder), but it might have its own issues to be looked into. Try it and see how it goes.

@nrc
Copy link
Collaborator

nrc commented Dec 9, 2015

I had a quick look through this and it (other than the HIR thing) these all seem like reasonable changes.

@potanin potanin merged commit b1baed3 into GSam:master Aug 7, 2016
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.

4 participants