This allows to remove using namespace llvm; in those *.cpp files
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 23911 Build 23910: arc lint + arc unit
Event Timeline
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 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::*