Skip to content

Lldb cleanups #19166

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 4 commits into from
Nov 23, 2014
Merged

Lldb cleanups #19166

merged 4 commits into from
Nov 23, 2014

Conversation

richo
Copy link
Contributor

@richo richo commented Nov 21, 2014

While poking at rust in lldb I found a few nits to clean up.

The file doesn't adhere to the python standard, but this will let vi do
The Right Thing by default
Be more idiomatic and rely less on fiddly construction of output
@alexcrichton
Copy link
Member

r? @michaelwoerister


if has_field_names:
output += " { \n"
template = "{\n%(type_name)s%(body)s\n}"
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it would put the type name inside the braces instead of in front of them.

@michaelwoerister
Copy link
Member

Apart from the comments above this looks good to me. If you fix these, please run make check-stage1-debuginfo-lldb to make sure that no tests get broken by the changes.

@richo
Copy link
Contributor Author

richo commented Nov 21, 2014

I'm fixing the issues now. The test harness looks broken on master. Poking now.

template = "%(type_name)s(%(body)s)"
separator = ", "

if not type_name.startswith("("):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like it's the same predicate as has_field_names, but I haven't dug far enough or thought hard enough about it to verify. If it is, this can be cleaned up further by just removing the $(type_name)s from the second template. Is that accurate?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's a bit different. Tuple structs have a type name but no field names, as in struct MyInt(int). The same is true for tuple-like enum variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, as opposed to a type alias for a tuple type, eg type Foobar = (int, f64)?

@richo
Copy link
Contributor Author

richo commented Nov 21, 2014

Ignore that, it's just namespaced enums fall out. Patch will go out on this PR in a sec.

@richo
Copy link
Contributor Author

richo commented Nov 21, 2014

Ok, the non ignored tests are all green, and the compiletest harness works (is there a reason this doesn't happen in CI? something something bors doesn't have debuggers handy?)

r? @michaelwoerister

@michaelwoerister
Copy link
Member

Thanks, @richo. This looks good to me now.
It's strange though that you had to do those changes in runtest.rs. The

#[cfg(not(stage0))]
use self::TargetLocation::*;

part at the top should have taken care of that. @alexcrichton, any idea what that's about?

@alexcrichton
Copy link
Member

Yeah that's fine, the stage0 compiler inserted that import automatically (essentially), so it's flagged as unused, but the stage1+ compiler doesn't do that, so we have to add it explicitly.

@michaelwoerister
Copy link
Member

Yeah, but even with the explicit use statement, these changes in runtest.rs were apparently needed for it to compile. Am I missing something here?

@alexcrichton
Copy link
Member

Oh now that I wouldn't expect!

@alexcrichton
Copy link
Member

It may just be some stage0/stage1 weirdness though, I wouldn't worry too too much about it.

@michaelwoerister
Copy link
Member

Thanks Alex! I'll try to give bors the OK on the PR.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 22, 2014
While poking at rust in lldb I found a few nits to clean up.
@bors bors merged commit 68f90a2 into rust-lang:master Nov 23, 2014
lnicola added a commit to lnicola/rust that referenced this pull request Feb 17, 2025
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