[llvm] r198351 - Fix up a couple of review comments:

[dblaikie at gmail.com]

I'd probably still be a bit more terse (no surprise there), omitting the
"if we're splitting dwarf, etc", that's implied by the context. Something
like "Create the skeleton up-front so its labels can be referenced (eg: in
arange emission)". Up to you if you want to tersify it in that sort of way.

Also in the "reasonable people may disagree" territory, I'd still consider
omitting the comment. It seems like a reasonable place to build the
skeleton, so I don't see that it needs justification. If code is obvious
enough, I wouldn't bother providing further justification, especially if
it'll crash/assert/fail to compile in a useful way if someone moves it to
the wrong place anyway.

On Thu Jan 02 2014 at 1:43:52 PM, Eric Christopher <echristo at gmail.com>
wrote:

I elaborated on the comment in r198358, let me know if that helps or
if it should still go away for you. I admit to liking more comments :)

-eric

On Thu, Jan 2, 2014 at 1:30 PM, Eric Christopher <echristo at gmail.com> wrote:
> Sure. I'll grab it here in a second.
>
> -eric
>
> On Thu, Jan 2, 2014 at 1:28 PM, dblaikie at gmail.com <dblaikie at gmail.com>
wrote:
>> It's more that the code seems fairly self documenting - saying what we're
>> doing seems useful when what we're doing is obscured by (presumably
>> unavoidable) complexity, but that particular line seems fairly
>> self-documenting.
>>
>>
>> On Thu Jan 02 2014 at 1:25:34 PM, Eric Christopher <echristo at gmail.com>
>> wrote:
>>>
>>> I guess it doesn't add anything more than "hey, we're doing this here"
>>> sort of documentation. I use those sorts of comments quite a bit, but
>>> understand if you don't want them.
>>>
>>> On Thu Jan 02 2014 at 1:16:58 PM, dblaikie at gmail.com <dblaikie at gmail.com
>
>>> wrote:
>>>
>>>
>>>
>>> On Thu Jan 02 2014 at 1:09:50 PM, Eric Christopher <echristo at gmail.com>
>>> wrote:
>>>
>>> Author: echristo
>>> Date: Thu Jan  2 15:03:28 2014
>>> New Revision: 198351
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=198351&view=rev
>>> Log:
>>> Fix up a couple of review comments:
>>>
>>> Use an if statement instead of a pair of ternary operators checking
>>> the same condition.
>>> Use a cheap method call rather than returning the local symbol.
>>>
>>> Modified:
>>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/
AsmPrinter/DwarfDebug.cpp?rev=198351&r1=198350&r2=198351&view=diff
>>>
>>> ============================================================
==================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Thu Jan  2 15:03:28
>>> 2014
>>> @@ -822,14 +822,14 @@ DwarfCompileUnit *DwarfDebug::constructD
>>>    if (!FirstCU)
>>>      FirstCU = NewCU;
>>>
>>> -  NewCU->initSection(
>>> -      useSplitDwarf() ?
>>> Asm->getObjFileLowering().getDwarfInfoDWOSection()
>>> -                      : Asm->getObjFileLowering().getD
warfInfoSection(),
>>> -      useSplitDwarf() ? DwarfInfoDWOSectionSym : DwarfInfoSectionSym);
>>> -
>>> -  // If we're splitting the dwarf then construct the skeleton CU now.
>>> -  if (useSplitDwarf())
>>> +  if (useSplitDwarf()) {
>>> +
>>> NewCU->initSection(Asm->getObjFileLowering().getDwarfInfoDWOSection(),
>>> +                       DwarfInfoDWOSectionSym);
>>> +    // If we're splitting the dwarf then construct the skeleton CU now.
>>>
>>>
>>> Not sure about you, but this comment doesn't seem to add a lot to me...
>>>
>>>
>>>      NewCU->setSkeleton(constructSkeletonCU(NewCU));
>>> +  } else
>>> +    NewCU->initSection(Asm->getObjFileLowering().getDwarfInfoSection(),
>>> +                       DwarfInfoSectionSym);
>>>
>>>    CUMap.insert(std::make_pair(DIUnit, NewCU));
>>>    CUDieMap.insert(std::make_pair(Die, NewCU));
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/
AsmPrinter/DwarfUnit.h?rev=198351&r1=198350&r2=198351&view=diff
>>>
>>> ============================================================
==================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h Thu Jan  2 15:03:28
2014
>>> @@ -185,7 +185,7 @@ public:
>>>    MCSymbol *getLocalSectionSym() const {
>>>      if (Skeleton)
>>>        return Skeleton->getSectionSym();
>>> -    return SectionSym;
>>> +    return getSectionSym();
>>>    }
>>>
>>>    MCSymbol *getSectionSym() const {
>>> @@ -198,7 +198,7 @@ public:
>>>    MCSymbol *getLocalLabelBegin() const {
>>>      if (Skeleton)
>>>        return Skeleton->getLabelBegin();
>>> -    return LabelBegin;
>>> +    return getLabelBegin();
>>>    }
>>>
>>>    MCSymbol *getLabelBegin() const {
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140102/36de00a5/attachment.html>