Enh, sure. I'll remove them for now and we'll see how it goes :)
-eric
On Thu Jan 02 2014 at 2:03:14 PM, dblaikie at gmail.com <dblaikie at gmail.com>
wrote:
> 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().getDwarfInfoSe
> ction(),
> >>> + 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/a3ffc154/attachment.html>