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.<br>
<br>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.<br>
<br><div>On Thu Jan 02 2014 at 1:43:52 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I elaborated on the comment in r198358, let me know if that helps or<br>
if it should still go away for you. I admit to liking more comments :)<br>
<br>
-eric<br>
<br>
On Thu, Jan 2, 2014 at 1:30 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
> Sure. I'll grab it here in a second.<br>
><br>
> -eric<br>
><br>
> On Thu, Jan 2, 2014 at 1:28 PM, <a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a> <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>> It's more that the code seems fairly self documenting - saying what we're<br>
>> doing seems useful when what we're doing is obscured by (presumably<br>
>> unavoidable) complexity, but that particular line seems fairly<br>
>> self-documenting.<br>
>><br>
>><br>
>> On Thu Jan 02 2014 at 1:25:34 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> I guess it doesn't add anything more than "hey, we're doing this here"<br>
>>> sort of documentation. I use those sorts of comments quite a bit, but<br>
>>> understand if you don't want them.<br>
>>><br>
>>> On Thu Jan 02 2014 at 1:16:58 PM, <a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a> <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>> On Thu Jan 02 2014 at 1:09:50 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>><br>
>>> wrote:<br>
>>><br>
>>> Author: echristo<br>
>>> Date: Thu Jan 2 15:03:28 2014<br>
>>> New Revision: 198351<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=198351&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>pr<u></u>oject?rev=198351&view=rev</a><br>
>>> Log:<br>
>>> Fix up a couple of review comments:<br>
>>><br>
>>> Use an if statement instead of a pair of ternary operators checking<br>
>>> the same condition.<br>
>>> Use a cheap method call rather than returning the local symbol.<br>
>>><br>
>>> Modified:<br>
>>> llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfDebug.cpp<br>
>>> llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfUnit.h<br>
>>><br>
>>> Modified: llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfDebug.cpp<br>
>>> URL:<br>
>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=198351&r1=198350&r2=198351&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>pr<u></u>oject/llvm/trunk/lib/<u></u>CodeGen/<u></u>AsmPrinter/DwarfDebug.<u></u>cpp?rev=<u></u>198351&r1=198350&r2=<u></u>198351&<u></u>view=diff</a><br>
>>><br>
>>> ==============================<u></u><u></u>==============================<u></u><u></u>==================<br>
>>> --- llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfDebug.cpp (original)<br>
>>> +++ llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfDebug.cpp Thu Jan 2 15:03:28<br>
>>> 2014<br>
>>> @@ -822,14 +822,14 @@ DwarfCompileUnit *DwarfDebug::constructD<br>
>>> if (!FirstCU)<br>
>>> FirstCU = NewCU;<br>
>>><br>
>>> - NewCU->initSection(<br>
>>> - useSplitDwarf() ?<br>
>>> Asm->getObjFileLowering().<u></u>getD<u></u>warfInfoDWOSection()<br>
>>> - : Asm->getObjFileLowering().<u></u>getD<u></u>warfInfoSection(),<br>
>>> - useSplitDwarf() ? DwarfInfoDWOSectionSym : DwarfInfoSectionSym);<br>
>>> -<br>
>>> - // If we're splitting the dwarf then construct the skeleton CU now.<br>
>>> - if (useSplitDwarf())<br>
>>> + if (useSplitDwarf()) {<br>
>>> +<br>
>>> NewCU->initSection(Asm-><u></u>getObj<u></u>FileLowering().<u></u>getDwarfInfoDWO<u></u>Section(),<br>
>>> + DwarfInfoDWOSectionSym);<br>
>>> + // If we're splitting the dwarf then construct the skeleton CU now.<br>
>>><br>
>>><br>
>>> Not sure about you, but this comment doesn't seem to add a lot to me...<br>
>>><br>
>>><br>
>>> NewCU->setSkeleton(<u></u>constructS<u></u>keletonCU(NewCU));<br>
>>> + } else<br>
>>> + NewCU->initSection(Asm-><u></u>getOb<u></u>jFileLowering().<u></u>getDwarfInfoSe<u></u>ction(),<br>
>>> + DwarfInfoSectionSym);<br>
>>><br>
>>> CUMap.insert(std::make_pair(<u></u>D<u></u>IUnit, NewCU));<br>
>>> CUDieMap.insert(std::make_<u></u>pai<u></u>r(Die, NewCU));<br>
>>><br>
>>> Modified: llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfUnit.h<br>
>>> URL:<br>
>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h?rev=198351&r1=198350&r2=198351&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>pr<u></u>oject/llvm/trunk/lib/<u></u>CodeGen/<u></u>AsmPrinter/DwarfUnit.<u></u>h?rev=<u></u>198351&r1=198350&r2=<u></u>198351&<u></u>view=diff</a><br>
>>><br>
>>> ==============================<u></u><u></u>==============================<u></u><u></u>==================<br>
>>> --- llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfUnit.h (original)<br>
>>> +++ llvm/trunk/lib/CodeGen/<u></u>AsmPrin<u></u>ter/DwarfUnit.h Thu Jan 2 15:03:28 2014<br>
>>> @@ -185,7 +185,7 @@ public:<br>
>>> MCSymbol *getLocalSectionSym() const {<br>
>>> if (Skeleton)<br>
>>> return Skeleton->getSectionSym();<br>
>>> - return SectionSym;<br>
>>> + return getSectionSym();<br>
>>> }<br>
>>><br>
>>> MCSymbol *getSectionSym() const {<br>
>>> @@ -198,7 +198,7 @@ public:<br>
>>> MCSymbol *getLocalLabelBegin() const {<br>
>>> if (Skeleton)<br>
>>> return Skeleton->getLabelBegin();<br>
>>> - return LabelBegin;<br>
>>> + return getLabelBegin();<br>
>>> }<br>
>>><br>
>>> MCSymbol *getLabelBegin() const {<br>
>>><br>
>>><br>
>>> ______________________________<u></u><u></u>_________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailm<u></u>an/listinfo/llvm-commits</a><br>
</blockquote>