[patch] Support multiple comdat sections with the same name but different groups

Rafael EspĂ­ndola rafael.espindola at gmail.com
Sat Oct 19 07:36:02 PDT 2013


>> Can we use a DenseMap instead of a std::map?
>
>
> Would require a bit more work - DenseMap can't handle a pair<string, string>
> key (no tombstone value), not sure if there's something I can do to address
> that.

You would probably need to create dedicated class for the pair and
implement densemapinfo for it. This is probably not too hot, lets keep
the std::map for now and benchmark it once the rest of the patch is
done.

>>
>> Can you add a new test and leave comdat.s unchanged? Would make it
>> easier to see that the new code behaves the same in the case of
>> distinct names.
>
>
> I tend to prefer avoiding adding new test files if possible (keeping testing
> related to the same feature together) to avoid excess process execution
> overhead.
>
> It seems fairly easy to split out examples of different names and the same
> name so they're all tested, but tested separately, in the same file.
>
> But I'm not wedded to the idea either.

At least for now please add a second test. Object file dumps are
somewhat unstable, which makes the test changes hard to read.

A followup patch doing nothing but merging the tests would probably be OK.

Cheers,
Rafael



More information about the llvm-commits mailing list