[PATCH] D64042: [Symbol] Improve Variable::GetLanguage

[Alex Langford via Phabricator via llvm-commits]

xiaobai marked an inline comment as done.
xiaobai added inline comments.


================
Comment at: lldb/trunk/source/Symbol/Variable.cpp:61-69
+  if (auto *func = m_owner_scope->CalculateSymbolContextFunction()) {
+    if ((lang = func->GetLanguage()) && lang != lldb::eLanguageTypeUnknown)
+      return lang;
+    else if (auto *comp_unit =
+                 m_owner_scope->CalculateSymbolContextCompileUnit())
+      if ((lang = func->GetLanguage()) && lang != lldb::eLanguageTypeUnknown)
+        return lang;
----------------
labath wrote:
> 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...
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64042/new/

https://reviews.llvm.org/D64042