shafik added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:402-403
case DW_AT_type:
- type = form_value;
+ if (!type.IsValid())
+ type = form_value;
break;
----------------
JDevlieghere wrote:
> What's the purpose of this? Do we expect to see the type attribute more than
> once?
Good question, the first iteration was done by @teemperor and then I modified
it heavily but I did not dig into this change to deeply but I was pretty sure
when we first talked about there was a good reason for it.
================
Comment at:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1337-1339
+ TypeSP ret_type = dwarf->FindTypeForAutoReturnForDIE(
+ die, ConstString(attrs.mangled_name));
+ if (ret_type) {
----------------
JDevlieghere wrote:
> LLVM likes to put these variables in the if-clause, which I personally really
> like because it conveys the scope without hurting readability.
Good catch, I was changing this code around a lot and forgot to go back and
clean this up.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2672-2675
+ TypeSP type_sp;
+
+ if (!name)
+ return type_sp;
----------------
JDevlieghere wrote:
> I know this pattern is common in LLDB, but I really dislike it because you
> have to backtrack all the way to the beginning of the function to know if
> `type_sp` has been modified in any way. When I write code like, this I tend
> to use `return {};` to make it clear I'm returning a default constructed
> instance of the return type. That also makes it clear where we're actually
> returning a non-default instance by just looking at the `return`s.
Good catch, I originally based this on `FindCompleteObjCDefinitionTypeForDIE`
and stuck with the idiom w/o thinking about it.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:406
+ virtual lldb::TypeSP
+ FindTypeForAutoReturnForDIE(const DWARFDIE &die,
----------------
shafik wrote:
> teemperor wrote:
> > If this is virtual then I guess `SymbolFileDWARFDwo` should overload it?
> I am actually not sure if this has to be virtual or not, let me dig into this
> and see if there would be any difference for `SymbolFileDWARFDwo` or not.
Since this is based on the same approach in `GetObjCClassSymbol` and that is
not `virtual` I am going to drop it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105564/new/
https://reviews.llvm.org/D105564
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
