This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Wrap things in `namespace lld { namespace elf {`, NFC
ClosedPublic

Authored by MaskRay on Oct 2 2019, 1:15 AM.

Event Timeline

MaskRay created this revision.Oct 2 2019, 1:15 AM
MaskRay edited the summary of this revision. (Show Details)Oct 2 2019, 1:15 AM
MaskRay updated this revision to Diff 222768.Oct 2 2019, 1:20 AM
MaskRay added reviewers: atanasyan, grimar, peter.smith, ruiu.

Add reviewers

grimar accepted this revision.Oct 2 2019, 1:29 AM

LG. Lets see what other people think.

This revision is now accepted and ready to land.Oct 2 2019, 1:29 AM
ruiu added a comment.Oct 2 2019, 1:35 AM

I think that somewhere in the Coding Style Guideline, it is recommended to keep namespace {} as narrow as possible. This is why I chose this style in the first place. Has that changed?

I think that somewhere in the Coding Style Guideline, it is recommended to keep namespace {} as narrow as possible. This is why I chose this style in the first place. Has that changed?

I don't know the history but https://llvm.org/docs/CodingStandards.html#id56 gives an example that does this:

namespace llvm {
namespace knowledge {
...

@ruiu Are you happy with this change?

ruiu added a comment.Oct 3 2019, 9:47 PM

Yeah, I'm fine with quoting the entire file with the namespaces, but I'm not entirely happy about adding endian:: to read32. Looks like the problem is that we use the same name for two functions. Can you just rename one to avoid the name collision? Does that make the code look simpler?

MaskRay added a comment.EditedOct 3 2019, 9:59 PM

Yeah, I'm fine with quoting the entire file with the namespaces, but I'm not entirely happy about adding endian:: to read32. Looks like the problem is that we use the same name for two functions. Can you just rename one to avoid the name collision? Does that make the code look simpler?

Adding endian:: is due to the rule of unqualified name lookup. Renaming either family of functions below will cause the change of hundreds of call sites changes.

llvm::support::endian::write32 and its friends
lld::elf::write32 and its friends

llvm::support::endian::* is used in very few files. And as this patch suggests, only Mips.cpp needs endian::*. I think this change clarifies things without making code more complex.

ruiu added a comment.Oct 3 2019, 10:06 PM

How about always using elf::write32 instead of endian::write32<E>?

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

ruiu added a comment.Oct 3 2019, 10:47 PM

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

I mean we can replace all occurrences of endian::write32<E>() with write32(). It looks like lld still compiles after I run the following command to replace them.

sed -i 's/write32<.>/write32/g' lld/ELF/Arch/Mips.cpp

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

I mean we can replace all occurrences of endian::write32<E>() with write32(). It looks like lld still compiles after I run the following command to replace them.

sed -i 's/write32<.>/write32/g' lld/ELF/Arch/Mips.cpp

This will make the generated code slightly worse because the implementation tests config->endianness. llvm::support::endian::write16(p, v, config->endianness);

Personally I prefer keeping the templated version, though I don't mind changing it if you are strong about this or @atanasyan is fine with lld::elf::* functions.

ruiu added a comment.Oct 3 2019, 11:10 PM

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

I mean we can replace all occurrences of endian::write32<E>() with write32(). It looks like lld still compiles after I run the following command to replace them.

sed -i 's/write32<.>/write32/g' lld/ELF/Arch/Mips.cpp

This will make the generated code slightly worse because the implementation tests config->endianness. llvm::support::endian::write16(p, v, config->endianness);

Personally I prefer keeping the templated version, though I don't mind changing it if you are strong about this or @atanasyan is fine with lld::elf::* functions.

I originally preferred the template version of write32 for the exact same reason, but I now prefer non-template write32 because it doesn't seem slow at all, perhaps because the branch is highly predictable. Every time it actually branches to the same direction. So maybe the template version is just making the code too verbose?

ruiu accepted this revision.Oct 7 2019, 12:52 AM

LGTM

MaskRay updated this revision to Diff 223457.Oct 7 2019, 1:25 AM
MaskRay edited the summary of this revision. (Show Details)

Update description. We don't change write* read* now