Convert rust_log.cpp to Rust, closes #8703 #8880

fhahn · 2013-08-30T14:55:38Z

I've started working on #8703.

RUST_LOG="::help" should work, I hope I'll be able to finish the rest this weekend.

brson · 2013-08-30T23:12:19Z

Thanks for tackling this! I love removing C++.

fhahn · 2013-08-31T15:14:00Z

Thanks for the feedbacl @brson

All functionality from rust_log.cpp should be ported to Rust now.

I've added a couple of unit tests as well.

The Rust implementation should give slightly better error messages than the c++ version, if a log level in RUST_LOG isn't a valid integer.

alexcrichton · 2013-08-31T19:46:21Z

src/libstd/rt/logging.rs

+    assert_eq!(dirs[2].level, 4);
+}
+
+#[cfg(test)]


Could you wrap all these tests in a #[cfg(test)] mod tests { ... } module? That way you don't need #[cfg(test)] on all the functions and it's also easy to see where the tests start and end.

#[test] implies #[cfg(test)], fwiw. Wrapping tests in a module is nice because it makes it easier to scope test-only uses.

I wanted to do that, but parse_loggin_spec and update_entry aren't public.

Is there a way to use private functions in a module in the same file?

alexcrichton · 2013-08-31T20:09:22Z

If you're feeling really brave while you're touching this code you could try knocking out #6031 or #6033, but that could also happen at a later time :)

fhahn · 2013-09-01T23:25:17Z

Thanks for the feedback @alexcrichton, I hope I addressed all your comments.

I could work on #6031 and #6033, but it seems like @novalis started working on them already?

novalis · 2013-09-02T05:26:56Z

Once this gets merged, I would be happy to reimplement my fix of #6031. I assume #6033 will be easy as well, and if so, I'll do that as well.

alexcrichton · 2013-09-03T04:28:23Z

src/libstd/rt/logging.rs

+}
+
+// This is the Rust representation of the mod_entry struct in src/rt/rust_crate_map.h
+struct ModEntry{


This is actually a really-dangerous version of the mod_entry struct. Because this is C FFI, we can't assume that char* == ~str, so I think that this would be more appropriately written as

struct ModEntry { name: *c_char, log_level: *u32, // note the '*', this is important for the log level to actually get set. }

One of the main problems as-is is that if you drop the structure (if it didn't materialize through an & pointer), it would attempt to free name, leading to lots of pain

Why does log_level have to be a pointer?

Shouldn't a normal value be enough, because its accessed and modified via a pointer to the struct?

That's what it's defined to be in the header file in C++. I think that we create a global in LLVM with some random name and then shove a pointer to the global in the mod_entry struct.

Oh I overlooked that!

Should be fixed now

alexcrichton · 2013-09-03T04:34:55Z

Also, I think it's best to hold of the extra issues for later pull requests. Looks like we've got those taken care of :)

fhahn · 2013-09-03T22:42:18Z

@alexcrichton thanks for the extensive feedback and your patience.

I've updated the pointer related stuff, but I had to use to_c_str and transmute to set up the test data (not sure if there's a nicer way to test it)

alexcrichton · 2013-09-04T02:54:20Z

No problem at all! Thanks for putting up with me :)

Once the ModEntry struct is changed, this looks good to go to me. The tests look fine to me (thanks for writing those!)

I've started working on #8703. RUST_LOG="::help" should work, I hope I'll be able to finish the rest this weekend.

alexcrichton · 2013-09-05T06:49:54Z

Thanks, and nice work!

fhahn · 2013-09-05T10:53:37Z

I was thinking about converting rust_crate_map.cpp to Rust as well. That would simplify src/libstd/rt/logging.rs as well, because we could get rid of the code for calling a rust closure in c++.

But I couldn't find the place were the crate map is initialized and that would be very helpful as a starting point.

huonw · 2013-09-05T11:01:36Z

It's actually static data in the executable that gets passed to the #[start] function, i.e. it has signature:

#[start]
fn start(argc: int, argv: **u8, crate_map: *u8) -> int

The crate map is created and added to the binary in rustc::middle::trans::base::decl_crate_map.

fhahn · 2013-09-07T13:59:49Z

Thanks for the hint, I'm looking into rewriting rust_crate_map.cpp and I'll open a pull request as soon as I have something presentable

This patch converts the rust_crate_map.cpp to Rust as mentioned at the end of #8880.

Add some testcases for recent rustfix update changelog: none This adds a testcase for a bugfix that has been fixed by https://github.com/rust-lang/rustfix/tree/v0.6.1 `rustfix` is pulled in by `compiletest_rs`. So to test that the correct rustfix version is used, I added one (and a half) testcase. I tried to add a testcase for rust-lang#8734 as well, but interesting enough the rustfix is wrong: ```diff fn issue8734() { let _ = [0u8, 1, 2, 3] .into_iter() - .and_then(|n| match n { + .flat_map(|n| match n { + 1 => [n + .saturating_add(1) 1 => [n .saturating_add(1) .saturating_add(1) .saturating_add(1) .saturating_add(1) .saturating_add(1) .saturating_add(1) .saturating_add(1) .saturating_add(1)], n => [n], }); } ``` this needs some investigation and then this testcase needs to be enabled by commenting it out closes rust-lang#8878 related to rust-lang#8734

alexcrichton reviewed Aug 31, 2013
View reviewed changes

alexcrichton mentioned this pull request Aug 31, 2013

Fix #6031 #8906

Closed

alexcrichton reviewed Sep 3, 2013
View reviewed changes

alexcrichton mentioned this pull request Sep 4, 2013

RUST_LOG=::help should be sorted #8949

Closed

Convert rust_log.cpp to Rust, closes rust-lang#8703

e38739b

bors added a commit that referenced this pull request Sep 4, 2013

auto merge of #8880 : fhahn/rust/issue_8703, r=brson

60fba4d

I've started working on #8703. RUST_LOG="::help" should work, I hope I'll be able to finish the rest this weekend.

bors closed this Sep 4, 2013

fhahn mentioned this pull request Sep 9, 2013

Convert rust_crate_map.cpp to Rust #9087

Closed

bors added a commit that referenced this pull request Sep 13, 2013

auto merge of #9087 : fhahn/rust/rust_crate_map, r=brson

323e8f0

This patch converts the rust_crate_map.cpp to Rust as mentioned at the end of #8880.

fhahn deleted the issue_8703 branch January 7, 2014 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert rust_log.cpp to Rust, closes #8703 #8880

Convert rust_log.cpp to Rust, closes #8703 #8880

fhahn commented Aug 30, 2013

brson commented Aug 30, 2013

fhahn commented Aug 31, 2013

alexcrichton Aug 31, 2013

sfackler Sep 1, 2013

fhahn Sep 1, 2013

alexcrichton commented Aug 31, 2013

fhahn commented Sep 1, 2013

novalis commented Sep 2, 2013

alexcrichton Sep 3, 2013

alexcrichton Sep 3, 2013

fhahn Sep 3, 2013

alexcrichton Sep 4, 2013

fhahn Sep 4, 2013

alexcrichton commented Sep 3, 2013

fhahn commented Sep 3, 2013

alexcrichton commented Sep 4, 2013

alexcrichton commented Sep 5, 2013

fhahn commented Sep 5, 2013

huonw commented Sep 5, 2013

fhahn commented Sep 7, 2013

Convert rust_log.cpp to Rust, closes #8703 #8880

Convert rust_log.cpp to Rust, closes #8703 #8880

Conversation

fhahn commented Aug 30, 2013

brson commented Aug 30, 2013

fhahn commented Aug 31, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 31, 2013

fhahn commented Sep 1, 2013

novalis commented Sep 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 3, 2013

fhahn commented Sep 3, 2013

alexcrichton commented Sep 4, 2013

alexcrichton commented Sep 5, 2013

fhahn commented Sep 5, 2013

huonw commented Sep 5, 2013

fhahn commented Sep 7, 2013