Page MenuHomePhabricator

[Symbol] Improve Variable::GetLanguage

Authored by xiaobai on Jul 1 2019, 3:34 PM.



When trying to ascertain what language a variable belongs to, just
checking the compilation unit is often not enough. In r364845 I added a way to
check for a variable's language type, but didn't put it in Variable itself.
Let's go ahead and put it in Variable.

Diff Detail


Event Timeline

xiaobai created this revision.Jul 1 2019, 3:34 PM

Why did you make this a function of Variable, rather than SymbolContextScope?

Why did you make this a function of Variable, rather than SymbolContextScope?

After thinking about it more, I realized that the implementation inside of SymbolContextScope would be identical or near identical to the implementation in SymbolContext. I thought that because IsRuntimeSupportValue requires that you have a Variable, that it would make sense to ask the Variable for its language. An alternative could be to ask the SymbolContext for its language instead, which I think will be effectively equivalent here.

I also noticed that there's a lot of repetitive code around asking for languages. Everybody always guesses from the mangled name and asks the compilation unit at some point during the process. Adding another implementation inside of SymbolContextScope would kind of add to the cruft a bit here I think.

jingham accepted this revision.Jul 22 2019, 10:24 AM

Sounds reasonable.

This revision is now accepted and ready to land.Jul 22 2019, 10:24 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 1:15 PM
labath added a subscriber: labath.Jul 23 2019, 3:15 AM
labath added inline comments.

I get a warning here (at least with gcc) about comp_unit variable being unused. Did you perhaps mean to do lang = comp_unit->GetLanguage() on the line below?

I would have fixed this myself, but when I started looking at this, I became unsure of that is this code exactly supposed to do. Is the check for the CompileUnit really supposed to be nested inside the check the existence of a function. I would have kind of expected it to be located outside the function if statement (if (func) stuff(func); else if (cu) stuff(cu);

I also find the if( lang = ... && lang != Unknown) pattern very confusing. As it stands now, it checks for the zero value twice (once due to the (lang = ...) part and once because eLanguageTypeUnknown is zero), but that part is very unobvious...

xiaobai marked an inline comment as done.Jul 23 2019, 10:34 AM
xiaobai added inline comments.

Yikes, yes, the logic for this change is very busted. I don't know what I was thinking, but I will upload a fix very soon.
I went with this pattern because I didn't want to assume eLanguageTypeUnknown is 0 but it's unlikely to change anytime soon.

will upload a fix. sorry about this.