[PATCH] D53407: [llvm-mca] Move namespace mca inside llvm::

Andrea Di Biagio via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 29 05:17:03 PDT 2018


andreadb added a comment.

In https://reviews.llvm.org/D53407#1274525, @MaskRay wrote:

> > 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


Repository:
  rL LLVM

https://reviews.llvm.org/D53407





More information about the llvm-commits mailing list