> On Mar 4, 2015, at 3:30 PM, Reid Kleckner <[email protected]> wrote:
> > + if (D.isFunctionDefinition() && CodeSegStack.CurrentValue &&
>
> D.isFunctionDefinition() is cheap, but CodeSegStack.CurrentValue is just as
> cheap and much more likely to short-circuit; you should check it first.
>
> Sure
> > + if (var->isThisDeclarationADefinition() &&
> > + ActiveTemplateInstantiations.empty()) {
> > + PragmaStack<StringLiteral *> *Stack = nullptr;
> > + int SectionFlags = PSF_Implicit | PSF_Read;
> > + if (var->getType().isConstQualified())
> > + Stack = &ConstSegStack;
> > + else if (!var->getInit()) {
> > + Stack = &BSSSegStack;
> > + SectionFlags |= PSF_Write;
> > + } else {
> > + Stack = &DataSegStack;
> > + SectionFlags |= PSF_Write;
> > + }
> > + if (!var->hasAttr<SectionAttr>() && Stack->CurrentValue)
>
> You should check Stack->CurrentValue first, it’s both cheaper and more likely
> to short-circuit.
>
> Sure
>
> 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.
>
> I think we want to come in here in !MicrosoftExt mode to do UnifySection.
> That helps us diagnose this:
> __attribute__((section(".mydata"))) extern const int x = 42;
> __attribute__((section(".mydata"))) int y = 42;
> t.cpp:2:41: error: 'y' causes a section type conflict with ‘x'
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.
Also, the code seems to be doing some redundant lookups of the section
attribute.
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.
Anyway, thanks for fixing this.
John.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
