<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div><blockquote type="cite" class=""><div class="">On Mar 4, 2015, at 3:30 PM, Reid Kleckner <<a href="mailto:rnk@google.com" class="">rnk@google.com</a>> wrote:</div><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">> + if (D.isFunctionDefinition() && CodeSegStack.CurrentValue &&<br class="">
<br class="">
</span>D.isFunctionDefinition() is cheap, but CodeSegStack.CurrentValue is just as<br class="">
cheap and much more likely to short-circuit; you should check it first.</blockquote><div class=""><br class=""></div><div class="">Sure</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">
> + if (var->isThisDeclarationADefinition() &&<br class="">
> + ActiveTemplateInstantiations.empty()) {<br class="">
> + PragmaStack<StringLiteral *> *Stack = nullptr;<br class="">
> + int SectionFlags = PSF_Implicit | PSF_Read;<br class="">
> + if (var->getType().isConstQualified())<br class="">
> + Stack = &ConstSegStack;<br class="">
> + else if (!var->getInit()) {<br class="">
> + Stack = &BSSSegStack;<br class="">
> + SectionFlags |= PSF_Write;<br class="">
> + } else {<br class="">
> + Stack = &DataSegStack;<br class="">
> + SectionFlags |= PSF_Write;<br class="">
> + }<br class="">
> + if (!var->hasAttr<SectionAttr>() && Stack->CurrentValue)<br class="">
<br class="">
</div></div>You should check Stack->CurrentValue first, it’s both cheaper and more likely to short-circuit.<br class=""></blockquote><div class=""><br class=""></div><div class="">Sure</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
In fact, you should consider just keeping a flag on Sema about whether *any* data section pragmas are active, since I assume that none are for the vast majority of time, and it would be good to avoid this entire block in that case. If you don’t do that, you should at least guard the block on getLangOpts().MicrosoftExt.<br class=""></blockquote><div class=""><br class=""></div><div class="">I think we want to come in here in !MicrosoftExt mode to do UnifySection. That helps us diagnose this:</div><div class=""><div class="">__attribute__((section(".mydata"))) extern const int x = 42;</div><div class="">__attribute__((section(".mydata"))) int y = 42;</div></div><div class=""><div class="">t.cpp:2:41: error: 'y' causes a section type conflict with ‘x'</div></div></div></div></div></div></blockquote><div><br class=""></div><div>Okay, but you could still trigger that check when applying an explicit section attribute instead of checking it for every variable. I know it would require a little bit of logic duplication for computing the flags, but I think it would be worth it.</div><div><br class=""></div><div>Also, the code seems to be doing some redundant lookups of the section attribute.</div><div><br class=""></div></div><div>Is this section-attributes conflict thing actually a problem for ELF, or is this COFF-specific? On Mach-O, this is already encoded in the section name, so it’s not really possible to have a conflict.</div><div><br class=""></div><div class="">Anyway, thanks for fixing this.</div><div class=""><br class=""></div><div class="">John.</div></body></html>