Page MenuHomePhabricator

[WebAssembly] Remove `using` statements from header files. NFC.
ClosedPublic

Authored by sbc100 on Nov 20 2018, 8:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Nov 20 2018, 8:21 AM

I think what @dschuff said in this comment was not removing all using from headers, but removing using namespace from headers.. no? We are not using using namespace in header files already.

I think what @dschuff said in this comment was not removing all using from headers, but removing using namespace from headers.. no? We are not using using namespace in header files already.

Hmm, good point. @ruiu, what is your take the policy for lld around this?

I think some redundant includes should definitely go (IWYU). Regarding more granular using declarations, honestly it's up to @ruiu, Clang and LLDB handle it by aliasing some of LLVM's common classes in their respective namespaces, maybe that could work for LLD too? In fact I'm guessing that's the purpose of lld/Common/LLVM.h, you could take a similar approach for Wasm specific stuff and limit the using statements to your namespace.

ruiu added a comment.Nov 26 2018, 9:51 AM

I think some redundant includes should definitely go (IWYU). Regarding more granular using declarations, honestly it's up to @ruiu, Clang and LLDB handle it by aliasing some of LLVM's common classes in their respective namespaces, maybe that could work for LLD too? In fact I'm guessing that's the purpose of lld/Common/LLVM.h, you could take a similar approach for Wasm specific stuff and limit the using statements to your namespace.

Your guess is correct; lld/Common/LLVM.h is to using some identifiers so that they can be referred without namespace specifiers. In fact, I think it is fine to add Wasm* names to that file because it is obvious that they will never conflict with other names because of Wasm prefix. Some names are ambiguous, such as llvm::object::Archive, and it is better to refer them using a fully qualified name, as we have other classes with confusing names (e.g. lld::*::ArchiveFile).

I think some redundant includes should definitely go (IWYU). Regarding more granular using declarations, honestly it's up to @ruiu, Clang and LLDB handle it by aliasing some of LLVM's common classes in their respective namespaces, maybe that could work for LLD too? In fact I'm guessing that's the purpose of lld/Common/LLVM.h, you could take a similar approach for Wasm specific stuff and limit the using statements to your namespace.

Your guess is correct; lld/Common/LLVM.h is to using some identifiers so that they can be referred without namespace specifiers. In fact, I think it is fine to add Wasm* names to that file because it is obvious that they will never conflict with other names because of Wasm prefix. Some names are ambiguous, such as llvm::object::Archive, and it is better to refer them using a fully qualified name, as we have other classes with confusing names (e.g. lld::*::ArchiveFile).

That does mean that lld/Common/LLVM.h will need to include llvm/Object/Wasm.h. It seems strange that has not been necessary for ELF/ COFF so far but I'll upload a new version this change and you can see what you think of it.

sbc100 updated this revision to Diff 175359.Nov 26 2018, 3:52 PM
  • feedback
ruiu accepted this revision.Nov 26 2018, 4:22 PM

LGTM. This is fine to me. We could create lld/wasm/LLVM.h to import Wasm* identifiers, but adding these identifiers directly to llvm/Common/LLVM.h makes more sense to me because the number of symbols we want to import is small.

That does mean that lld/Common/LLVM.h will need to include llvm/Object/Wasm.h. It seems strange that has not been necessary for ELF/ COFF so far but I'll upload a new version this change and you can see what you think of it.

As you noticed, you don't actually have to include llvm/Object/Wasm.h. You just need to declare class/struct names in lld/Common/LLVM.h.

include/lld/Common/LLVM.h
54 ↗(On Diff #175360)

nit: add a blank line before a new namespace declaration.

This revision is now accepted and ready to land.Nov 26 2018, 4:22 PM
This revision was automatically updated to reflect the committed changes.