This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Move namespace mca inside llvm::
ClosedPublic

Authored by MaskRay on Oct 18 2018, 11:50 AM.

Details

Summary

This allows to remove using namespace llvm; in those *.cpp files

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Oct 18 2018, 11:50 AM

Friendly ping :)

Friendly ping :)

Hi!

Thanks for the patch.

I am okay with this change provided that you only change files in llvm-mca/include or in llvm-mca/lib.
There is a plan to move the core abstractions of llvm-mca into llvm/lib (at some point; hopefully in the near future). So, files that are not part of the library (for example, Views) should not be touched.

Also, if you are going to move namespace mca under namespace llvm, then please update the code by removing all the redundant llvm:: prefixes.

Thanks,
-Andrea

I am okay with this change provided that you only change files in llvm-mca/include or in llvm-mca/lib.

Why is the difference between llvm-mca/include/* and llvm-mca/Views/*?

If llvm-mca/Views/* names want to stay in namespace mca, as they reference llvm-mca/include/*, they need to be written as:

namespace mca {
using namespace llvm::mca;

or use qualified llvm::mca::* (which is clumsy)

Having both namespace mca and namespace llvm::mca will make it easy to cause name conflict.

I am okay with this change provided that you only change files in llvm-mca/include or in llvm-mca/lib.

Why is the difference between llvm-mca/include/* and llvm-mca/Views/*?

If llvm-mca/Views/* names want to stay in namespace mca, as they reference llvm-mca/include/*, they need to be written as:

namespace mca {
using namespace llvm::mca;

or use qualified llvm::mca::* (which is clumsy)

Having both namespace mca and namespace llvm::mca will make it easy to cause name conflict.

I am okay with having the core abstractions defined inside namespace llvm::mca.
I am not a big fan of having all the driver components (like views) defined in namespace llvm::mca too. If the ultimate goal is just to remove a bunch of "using namespace" from a few files, then this patch looks like a too big change for very little gain.

That being said. I am not strongly against this change. So, I spoke with other people about your patch. We definitely want to move the core abstractions of mca inside namespace llvm; that would simplify the transition from the tools/mca directory to llvm/lib. We all agree that (at least conceptually) it is much nicer if driver abstractions don't live inside of namespace llvm. However, nobody felt strongly about it either.

In conclusion. It is fine if you want to move everything under llvm::mca. However, we may revisit this decision in future.

Please rebase your original patch, and remove all the redundant llvm:: prefixes from header files when possible.

Thanks,
Andrea

I am okay with this change provided that you only change files in llvm-mca/include or in llvm-mca/lib.

Why is the difference between llvm-mca/include/* and llvm-mca/Views/*?

If llvm-mca/Views/* names want to stay in namespace mca, as they reference llvm-mca/include/*, they need to be written as:

namespace mca {
using namespace llvm::mca;

or use qualified llvm::mca::* (which is clumsy)

Having both namespace mca and namespace llvm::mca will make it easy to cause name conflict.

I am okay with having the core abstractions defined inside namespace llvm::mca.
I am not a big fan of having all the driver components (like views) defined in namespace llvm::mca too. If the ultimate goal is just to remove a bunch of "using namespace" from a few files, then this patch looks like a too big change for very little gain.

That being said. I am not strongly against this change. So, I spoke with other people about your patch. We definitely want to move the core abstractions of mca inside namespace llvm; that would simplify the transition from the tools/mca directory to llvm/lib. We all agree that (at least conceptually) it is much nicer if driver abstractions don't live inside of namespace llvm. However, nobody felt strongly about it either.

In conclusion. It is fine if you want to move everything under llvm::mca. However, we may revisit this decision in future.

Please rebase your original patch, and remove all the redundant llvm:: prefixes from header files when possible.

Thanks,
Andrea

Thanks for the feedback.

Rebased. I decide to go with the easy route: moving the whole namespace mca inside llvm:: , the justification is

  • Having both llvm::mca mca cause namespace lookup ambiguity for mca::
  • In driver components' header files, I would otherwise use llvm::mca::* (core abstractions) here and there

When we want to revisit the decision (everything in llvm::mca::*) in the future, we can move things to a nested namespace of llvm::mca::, to conceptually make them separate from the rest of llvm::mca::*

andreadb accepted this revision.Oct 30 2018, 4:09 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 30 2018, 4:09 AM
This revision was automatically updated to reflect the committed changes.