typedef struct {} name #518
Conversation
|
|
There was a problem hiding this comment.
Nice! Was a lot easier than what I expected, good catch with the type name! :)
Note that this only works because of this line, but I guess we can rely on it fairly well.
I have a few minor comments, but this looks good to me, thanks again! :)
| if name.is_empty() { | ||
| // The pretty-printed name may contain typedefed name, | ||
| // but may also be "struct (anonymous at .h:1)" | ||
| let pretty_name = ty.spelling(); |
There was a problem hiding this comment.
Hey, good find with this, I thought this was going to be a lot trickier and that we were going to need to look back and forth for the correct typedef :)
| // The pretty-printed name may contain typedefed name, | ||
| // but may also be "struct (anonymous at .h:1)" | ||
| let pretty_name = ty.spelling(); | ||
| if Self::is_valid_identifier(&pretty_name) { |
There was a problem hiding this comment.
This feels somewhat hacky... But I guess it's ok.
This kind of overlaps with similar code in is_invalid_named_type. Can we reuse this function there?
|
Oh, one last request, can you test this works inside C++ namespaces? You can re-use |
|
I'm just wary that the name is going to be namespace::bar instead of just bar, and we're going to need to complicate this a bit. |
|
Yes, in C++ it's something like Only C has struct and union names in a separate namespace from other type names, so only C needs |
src/ir/ty.rs
Outdated
| } | ||
| _ => false, | ||
| } | ||
| } | ||
| pub fn is_invalid_name(name: &str) -> bool { | ||
| let mut chars = name.chars(); | ||
| let first = chars.next().unwrap(); |
There was a problem hiding this comment.
The fact that name is non-empty holds for Named types because we enforce it at creation, but does not necessarily hold for all inputs we provide to this, so I'd rather keep something similar to what you had:
pub fn is_valid_identifier(name: &str) -> bool {
let mut chars = name.chars();
chars.next().map_or(false, char::is_alphabetic) && chars.all(|c| c == '_' || c.is_alphanumeric())
}Then use !Self::is_valid_identifier in the Named case.
src/ir/ty.rs
Outdated
| @@ -1077,7 +1080,7 @@ impl Type { | |||
|
|
|||
| if name.is_empty() { | |||
| let pretty_name = ty.spelling(); | |||
| if Self::is_valid_identifier(&pretty_name) { | |||
| if !Self::is_invalid_name(&pretty_name) { | |||
There was a problem hiding this comment.
Regarding C++:
Yes, in C++ it's something like (anonymous namespace)::foo. However, I don't think this feature is useful in C++, so it's not a problem.
I kind of disagree, since I've definitely seen similar usage in Gecko, but as long as it doesn't break awfully (it just keeps pre-existing functionality) I think it's fine :).
Could you leave a comment either here or in the Record case to explicitly mention it doesn't work in C++ and point to the issue where other approaches were discussed? (those would actually work in C++).
kornelski
force-pushed
the
typedefstruct
branch
from
24697b9
to
4a14681
Compare
|
@bors-servo r+ Thanks! |
|
📌 Commit 70c61e1 has been approved by |
typedef struct {} name
Fixes #427
It looks like clang is doing the hard work of getting the right name from the typedef, but it falls back to arbitrary pretty-printed descriptions if it can't find a typedef. I couldn't find an API to check whether the name comes from a typedef, so I just filter out non-ident-like spellings.
|
☀️ Test successful - status-travis |
Fixes #427
It looks like clang is doing the hard work of getting the right name from the typedef, but it falls back to arbitrary pretty-printed descriptions if it can't find a typedef. I couldn't find an API to check whether the name comes from a typedef, so I just filter out non-ident-like spellings.